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

feat: add navigation confirmation when builder fields are dirty #4670

Merged
merged 24 commits into from
Sep 5, 2022

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Aug 23, 2022

Problem

This PR adds a navigation confirmation on navigation and change of field when the field being currently edited is dirty. The possible navigation cases are covered below:

  • start page
  • end page
  • field
  • builder tabs
  • drawer close button
  • drawer back button

Any permutation (start page -> field, field -> field, end page -> start page) will trigger a render of a new DirtyModal component if the fields are dirty. This was achieved by storing pending states in the respective stores and context, subscribing to a newly exposed isDirty selector in fieldBuilderStore.

Any route navigation change will also trigger a similar NavigationPrompt modal.

Closes #4304

Solution

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • feat: add NavigationPrompt modal component to show navigation confirmation modal when condition is true
    • this is used in this PR when the isDirty constant is true.
  • feat: update useDesignStore, useFieldBuilderStore, CreatePageSidebarContext to be responsive to dirty field state.
  • feat: show confirmation modal when navigating or changing edit fields when current field is dirty.

Screenshots

CreatePage stories should now have the behaviour, but here is a gif in case that is hard to navigate.
Recording 2022-08-25 at 12 06 00

@karrui karrui changed the title feat: add navigation confirmation when builder fields are dirtyForm v2/fix issue 4304 confirm navigation feat: add navigation confirmation when builder fields are dirty Aug 23, 2022
Base automatically changed from form-v2/navigation-tabs-template to form-v2/develop August 25, 2022 04:25
@karrui karrui force-pushed the form-v2/fix-issue-4304-confirm-navigation branch from a7ab3c5 to 84b378a Compare August 25, 2022 04:26
@justynoh
Copy link
Contributor

Regarding the UX itself, is there any particular reason to have different modals in each case? Since there are two sets of modals (one for navigation and one for changing the active state within the same route), I wonder if users would have the (incorrect) impression that "discarding changes" means different things for each modal.

For instance (and this is just my intuition - feel free to comment if you don't think so), if i click out of a dirty field, I might think that the "You have unsaved changes" modal will discard my changes to the current active field. But if let's say i've been working on my form and I try to navigate away, seeing a different modal ("Discard changes?") I might be worried that there are other changes to the form that aren't yet saved.

@karrui
Copy link
Contributor Author

karrui commented Aug 26, 2022

Regarding the UX itself, is there any particular reason to have different modals in each case? Since there are two sets of modals (one for navigation and one for changing the active state within the same route), I wonder if users would have the (incorrect) impression that "discarding changes" means different things for each modal.

For instance (and this is just my intuition - feel free to comment if you don't think so), if i click out of a dirty field, I might think that the "You have unsaved changes" modal will discard my changes to the current active field. But if let's say i've been working on my form and I try to navigate away, seeing a different modal ("Discard changes?") I might be worried that there are other changes to the form that aren't yet saved.

@pearlyong?

…vigation

# Conflicts:
#	frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignContent/FieldRow/FieldRowContainer.tsx
@pearlyong
Copy link

agree it might be confusing, let's stick with the 'You have unsaved changes' modal, which is easier English to understand

Copy link
Contributor

@justynoh justynoh left a comment

Choose a reason for hiding this comment

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

lgtm

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.

[1] Confirmation modal when navigating out of form field editing before saving
3 participants