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

Feature/4798 custom confirmation modal #4814

Merged
merged 11 commits into from
Nov 18, 2024

Conversation

robinmolen
Copy link
Contributor

Closes #4798

Changes

Replacing all window.confirm(...) confirmations with a new custom React hook (useConfirm). This new hook is using our Modal component to display the confirmation message. These changes happend to keep the application UI uniform.

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@robinmolen robinmolen linked an issue Nov 8, 2024 that may be closed by this pull request
@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from 3e198d4 to 4aaa7cc Compare November 8, 2024 10:47
@robinmolen robinmolen marked this pull request as ready for review November 8, 2024 10:53
Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.57%. Comparing base (51f9713) to head (022d950).
Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4814   +/-   ##
=======================================
  Coverage   96.57%   96.57%           
=======================================
  Files         748      748           
  Lines       25448    25448           
  Branches     3369     3369           
=======================================
  Hits        24576    24576           
  Misses        608      608           
  Partials      264      264           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stevenbal
Copy link
Contributor

@robinmolen can you fix the merge conflicts? Also not sure if the failing tests are related to the code changes?

@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch 2 times, most recently from e1881c9 to 231063c Compare November 11, 2024 11:20
@robinmolen
Copy link
Contributor Author

There is a new window.confirm added in a recent commit, in the /form_design/form-creation-form.js. I'll replace that one now with the new useConfirm

@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from 231063c to 9dbb416 Compare November 11, 2024 11:39
@robinmolen
Copy link
Contributor Author

I've replaced the .../form_design/form-creation-form.js window.confirm with the useConfirm. With that, all window.confirm's have been replaced :)

@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch 3 times, most recently from c5b04b4 to 3c47cb5 Compare November 14, 2024 09:23
robinmolen added a commit that referenced this pull request Nov 14, 2024
Rendering a component inside a hook has some serious performance impacts. To prevent this, the useConfirm now returns the needed props and a reference to the confirmationModal component.

For more info: #4814 (comment)
robinmolen added a commit that referenced this pull request Nov 14, 2024
Rendering a component inside a hook has some serious performance impacts. To prevent this, the useConfirm now returns the needed props and a reference to the confirmationModal component.

For more info: #4814 (comment)
@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from 477649e to 115e00e Compare November 14, 2024 15:57
robinmolen added a commit that referenced this pull request Nov 14, 2024
Rendering a component inside a hook has some serious performance impacts. To prevent this, the useConfirm now returns the needed props and a reference to the confirmationModal component.

For more info: #4814 (comment)
@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from 115e00e to d7217e1 Compare November 14, 2024 15:58
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

Holding of a little longer because of the SDK context/decorator for storybook I mentioned on Slack. I think it would clean up the new/updated interaction tests, and I'm curious if that formik onChange stuff is still necessary, since what it currently does is stop tracking the isTouched state and once we refactor to not roll our validation errors/mechanics 100% on our own, that will definitely cause some problems then :)

@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from d7217e1 to 720701a Compare November 18, 2024 10:30
robinmolen added a commit that referenced this pull request Nov 18, 2024
Rendering a component inside a hook has some serious performance impacts. To prevent this, the useConfirm now returns the needed props and a reference to the confirmationModal component.

For more info: #4814 (comment)
@robinmolen robinmolen force-pushed the feature/4798-custom-confirmation-modal branch from 720701a to 022d950 Compare November 18, 2024 10:38
Copy link
Member

@sergei-maertens sergei-maertens left a comment

Choose a reason for hiding this comment

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

bellissimo

@sergei-maertens sergei-maertens merged commit 5042ea1 into master Nov 18, 2024
33 of 34 checks passed
@sergei-maertens sergei-maertens deleted the feature/4798-custom-confirmation-modal branch November 18, 2024 11:44
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.

Using custom confirmation modals
3 participants