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.

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

Confirmation Dialog #653

Merged
merged 9 commits into from
Sep 15, 2022
Merged

Confirmation Dialog #653

merged 9 commits into from
Sep 15, 2022

Conversation

HenrikeW
Copy link
Contributor

@HenrikeW HenrikeW commented Sep 15, 2022

Related issue(s) and PR(s)

This PR closes #635.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

List of changes made

  • A reusable Alert component was created. It can either be rendered without children and take a simple question as a string prop (e.g. "Are you sure?"). Or it can be rendered with children (like date pickers) to include more content. Thus, it should be usable even in the future to implement the edit absence functionality.
  • For the specific case of action-confirmation, a confirmation context was created. This way all logic needed for showing/hiding the dialog and handling the button actions lives inside the context instead of in the calling component (in this case AbsencePlanner). The changes in AbsencePlanner are minimal and the confirmation dialog can easily be reused at any other place in the application.

Screenshot of the fix

image

Testing

  • Go to the Absence page, make sure you have planned vacations and try to delete one of them. You should be prompted the dialog.
  • "Yes" should delete the period, "Cancel" should close the dialog. Clicking Escape should also close the dialog.
  • Try to use keyboard navigation. When opening the dialogue, the focus should be on the cancel button by default.

Further comments

Definition of Done checklist

  • I have made an effort making the commit history understandable
  • I have performed a self-review of my own code and commented any hard-to-understand areas
  • Tests and lint/format validations are passing
  • My changes generate no new warnings

@HenrikeW HenrikeW force-pushed the dev/confirmation-dialogue branch from f1141a4 to cf6fb77 Compare September 15, 2022 08:54
Copy link
Contributor

@jonandernovella jonandernovella left a comment

Choose a reason for hiding this comment

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

It works! I left some feedback about the name of the Dialog component, which in my view should not be called Alert.

@jonandernovella jonandernovella self-requested a review September 15, 2022 10:07
@jonandernovella jonandernovella merged commit b1ea27b into develop Sep 15, 2022
@jonandernovella jonandernovella deleted the dev/confirmation-dialogue branch September 15, 2022 10:08
@jonandernovella jonandernovella mentioned this pull request Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

confirmation dialogue
2 participants