Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement data entry abort modal behaviour #256

Merged
merged 13 commits into from
Aug 16, 2024
Merged

Implement data entry abort modal behaviour #256

merged 13 commits into from
Aug 16, 2024

Conversation

praseodym
Copy link
Contributor

Implements most of #212, but not completely because of dependencies on #133.

Also fixes the abort button design as mentioned in #101.

Please review per commit for clarity.

@praseodym
Fix abort button styling according to design
af97719
@praseodym
Add endpoint to delete an in-progress data entry
bc93a67
@praseodym
Clean up frontend API client
1e3f4df 
And add support for DELETE requests.
@praseodym
Implement data entry abort modal behaviour
5dc14e9 
- Move abort button and model to separate component
- Implement delete data entry API call
- Implement abort modal save/delete behaviour
@praseodym praseodym added backend Issues or pull requests that relate to the backend frontend Issues or pull requests that relate to the frontend tests Issues or pull requests that relate to tests labels Aug 12, 2024
@praseodym praseodym self-assigned this Aug 12, 2024
@praseodym praseodym requested review from rnijveld, lkleuver and cikzh and removed request for ring-ring-ring August 12, 2024 16:01
Lionqueen94
Lionqueen94 previously approved these changes Aug 12, 2024
View reviewed changes
jorisleker
jorisleker requested changes Aug 13, 2024
View reviewed changes
jorisleker
jorisleker approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@jorisleker jorisleker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested:

  • created data entry, checked in database: saved
  • aborted + saved data entry: checked in database: still there
  • aborted + not saved: checked in database: deleted

jorisleker
jorisleker previously approved these changes Aug 13, 2024
View reviewed changes
@praseodym praseodym dismissed stale reviews from jorisleker and Lionqueen94 via b1bfd74 August 13, 2024 15:31
@praseodym
Copy link
Contributor Author

@jorisleker I've changed the CSS units from px to rem in b1bfd74.

jorisleker
jorisleker previously approved these changes Aug 13, 2024
View reviewed changes
@praseodym praseodym disabled auto-merge August 14, 2024 13:55
@praseodym praseodym force-pushed the 212-implement-data-entry-abort-modal-functionality branch from be7a375 to 0a59ba3 Compare August 14, 2024 14:02
@praseodym
Copy link
Contributor Author

It's a bit odd that it's possible to save your input, but only in #137 we will let you resume data entry. In that sense it would have been better to only implement the "abort" functionality now. Since we already had the placeholder for the dialog and #137 is the next epic in options, best to proceed as we are.

Probably something to mention at the start of the September demo, depending on where we are with #137 by then.

The original plan was to implement the abort functionality in #137. Issue #212 was created before the last biweekly planning session after discussion with @jorisleker. He proposed to only implement the "Niet opslaan" route.

@praseodym praseodym enabled auto-merge August 14, 2024 14:11
@praseodym praseodym requested review from cikzh and jschuurk-kr August 14, 2024 14:11
@praseodym praseodym added this pull request to the merge queue Aug 16, 2024
Merged via the queue into main with commit 8b925a3 Aug 16, 2024
7 checks passed
@praseodym praseodym deleted the 212-implement-data-entry-abort-modal-functionality branch August 16, 2024 08:48
This was referenced Aug 16, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkleuver lkleuver lkleuver approved these changes

@jorisleker jorisleker jorisleker approved these changes

@rnijveld rnijveld Awaiting requested review from rnijveld

@Lionqueen94 Lionqueen94 Awaiting requested review from Lionqueen94

@cikzh cikzh Awaiting requested review from cikzh

@jschuurk-kr jschuurk-kr Awaiting requested review from jschuurk-kr

Assignees

@praseodym praseodym

Labels
backend Issues or pull requests that relate to the backend frontend Issues or pull requests that relate to the frontend tests Issues or pull requests that relate to tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@praseodym @lkleuver @jschuurk-kr @cikzh @Lionqueen94 @jorisleker