square / maker

Maker Design System by Square
https://square.github.io/maker/styleguide/latest-stable/
Other
63 stars 14 forks source link

fix(modal): closing bugs #454

Closed pretzelhammer closed 1 year ago

pretzelhammer commented 2 years ago

Describe the problem this PR addresses

The bugs:

❌ (1) a beforeClose hook set via options would never run

One way to set a beforeClose hook was via the options object in the 2nd param of modalApi.open, e.g.

this.modalApi.open(
  // 1st param, render function
  () => <MyModal />,
  // 2nd param, modalApi options object
  {
    beforeCloseHook: () => console.log('I will never run 🀦'),
  },
);

This is bad obviously because the whole purpose of the beforeClose hook is to potentially block the modal from closing if the user forgot or failed to do something, but it would close regardless potentially leading to bad user experience or data in a corrupt or incomplete state. The beforeClose hook technically could also be used to perform final data clean up steps after a user was done interacting with a modal, and it failing to run could also lead to corrupt or incomplete data state.

Nobody noticed it was broken because... nobody was using it, which begs the question: why was it implemented in the first place 🀷? It was likely used for a time, then dropped or refactored, and then broken in later commit.

❌ (2) setting a beforeClose hook via the Modal's beforeClose prop would overwrite the beforeClose hook set via options

This is a very subtle bug that no one would have ever likely noticed because of bug (1) but even if that bug was fixed in an earlier or later version of Maker then this bug would pop up, e.g.

this.modalApi.open(
  // hook set via Modal prop
  () => <MyModal
    beforeClose={() => console.log('this hook overwrites the one below')}
  />,
  // hook set via modalApi options
  {
    beforeCloseHook: () => console.log("I'll be overwritten by propHook"),
    // this hook will never run 🀦
  },
);

❌ (3) calling the close function returned by modalApi.open would force close an entire stack of modals from the bottom

Calling modalApi.close is suppose to only close the current modal, and if there is another modal stacked on top of the current modal then closing is blocked. This was correctly enforced for modalApi.close but not correctly enforced for the close function returned from modalApi.open, e.g.

const closeFn = this.modalApi.open(() => <MyModal />);
closeFn(); // this could possibly force close multiple modals

Note: force closing a stack of modals from the bottom is not good because: the bottommost modal usually isn't aware of the state of the modals above it, and if the user is performing some action in the upper modals like filling out a form then randomly closing it on them is horrible user experience. This is why we encourage and attempt to enforce closing modals from the top only since the topmost modals have the most context and are the ones which are guiding the user experience.

❌ (4) calling the close function returned by modalApi.open would not run any beforeClose hooks on the closed modal

const closeFn = this.modalApi.open(
  () => <MyModal beforeClose={hookOne} />,
  {
    beforeCloseHook: hookTwo,
  },
);
closeFn(); // this would skip running hookOne & hookTwo 🀦

❌ (5) calling modalApi.state.parentModal.close no longer works, but even when it "worked" it was broken in the same way as described by bug (4)

Originally modalApi.state.parentModal.close was implemented to close a stack of modals from the top. Since then it broke during some commit and stopped working, however even when it did "work" it was still broken in the same way as described by bug (4), i.e. none of the beforeClose hook would run.

❌ (6) calling modalApi.state.parentModal.close would only close a stack of exactly 2 modals, but would break for any other number, e.g. 1 or 3+

I think it's okay to want to close an entire stack of modals from the top. However, the way to do it should be reliable and independent regardless of how many modals there are in the stack, e.g. 1, 2, 3, 4 and so on. The original implementation of this feature only enabled users to close a stack of exactly 2 modals, and for any other number it was useless.

Describe the changes in this PR

The fixes:

βœ… (1) beforeClose hook set via options now runs

this.modalApi.open(
  () => <MyModal />,
  {
    beforeCloseHook: () => console.log('Yay, I run now! πŸ™Œ'),
  },
);

βœ… (2) beforeClose hook set via Modal's beforeClose prop no longer overwrites the beforeClose hook set via options, both are run

this.modalApi.open(
  // hook set via Modal prop
  () => <MyModal
    beforeClose={() => console.log('I will run first! πŸ™Œ')}
  />,
  // hook set via modalApi options
  {
    beforeCloseHook: () => console.log('I will run second! πŸ™Œ'),
    // this hook will never run 🀦
  },
);

Both hooks have the opportunity to block closing. The hook set via props is run first and the hook set via options is run second. The hooks are run lazily, i.e. if the first hook blocks closing then the second hook isn't run.

βœ… (3) calling the close function returned by modalApi.open will at most close the single specific opened modal, but will not close if there is a stack of multiple modals

It's not possible to accidentally or unintentionally close a stack of modals from the bottom anymore, as the close function returned from modalApi.open will now only close that specific single opened modal, and if there are modals stacked on top of it then closing will fail and the function will return false, e.g.

const closeFn = this.modalApi.open(() => <MyModal />);
const result = closeFn();
// if result is true then MyModal was closed,
// if result is false then MyModal was not closed,
// which may mean there are other modals stacked
// on top of it which must be closed first

βœ… (4) calling the close function returned by modalApi.open will run all beforeClose hooks registered on the opened modal

const closeFn = this.modalApi.open(
  () => <MyModal beforeClose={hookOne} />,
  {
    beforeCloseHook: hookTwo,
  },
);
// this will now run hookOne & hookTwo before closing πŸ™Œ 
const result = await closeFn();
// if result is true then MyModal was closed,
// if result is false then MyModal was not closed,
// which may mean at least one of the beforeClose
// hooks blocked closing, i.e. returned false
// NOTE: if all of the hooks are sync then result will be a
// boolean, if any are async then result will be a Promise<boolean>

βœ… (5) you can now call modalApi.closeAll to close an entire stack of modals from the top, and it will correctly run all of the beforeClose hooks on each modal

// inside some part of the app
this.modalApi.open(
  () => <MyModal beforeClose={hookOne} />,
  {
    beforeCloseHook: hookTwo,
  },
);
// inside MyModal
this.modalApi.open(
  () => <MyNestedModal beforeClose={hookThree} />,
  {
    beforeCloseHook: hookFour,
  },
);
// inside MyNestedModal
const result = await this.modalApi.closeAll();
// hookThree & hookFour are run, if both return true MyNestedModal is closed
// hookOne & hookTwo are run, if both return true MyModal is closed
// if any of the hooks return false closing is stopped at that modal
// if all of the hooks are sync then result will be a boolean, if any of
// the hooks are async then result will be a Promise<boolean>

βœ… (6) calling modalApi.closeAll works for closing a stack of modals of any size

modalApi.closeAll works for closing a single modal, or a couple modals, or 3, or 4, or so on.

Other information

github-actions[bot] commented 2 years ago

Deployed Styleguide and Lab.

Notes
  1. Links may take a few minutes to update after PR is opened or commit is pushed.
  2. Links may become invalidated after PR is merged or closed.
  3. Links for all releases and open PRs can be found on the Maker Deploys page.

github-actions[bot] commented 2 years ago

πŸ“Š Package size report   0.7%↑

File Before After
components/Modal/script.js 10.2 kB 8%↑11.0 kB
Total (Includes all files) 1.7 MB 0.7%↑1.8 MB
Tarball size 331.3 kB 1%↑336.0 kB
Unchanged files | File | Size | | ------------------------------------------------------------------------------------ | --------: | | `components/Accordion/index.js` | `46 B` | | `components/Accordion/script.js` | `5.2 kB` | | `components/Accordion/styles.css` | `235 B` | | `components/ActionBar/index.js` | `46 B` | | `components/ActionBar/script.js` | `14.6 kB` | | `components/ActionBar/styles.css` | `5.7 kB` | | `components/Badge/index.js` | `46 B` | | `components/Badge/script.js` | `5.9 kB` | | `components/Badge/styles.css` | `1.8 kB` | | `components/Blade/index.js` | `46 B` | | `components/Blade/script.js` | `6.9 kB` | | `components/Blade/styles.css` | `837 B` | | `components/Button/index.js` | `46 B` | | `components/Button/script.js` | `8.4 kB` | | `components/Button/styles.css` | `6.0 kB` | | `components/Calendar/index.js` | `46 B` | | `components/Calendar/script.js` | `7.4 kB` | | `components/Calendar/styles.css` | `2.1 kB` | | `components/Card/index.js` | `46 B` | | `components/Card/script.js` | `2.4 kB` | | `components/Card/styles.css` | `603 B` | | `components/Checkbox/index.js` | `46 B` | | `components/Checkbox/script.js` | `3.9 kB` | | `components/Checkbox/styles.css` | `1.4 kB` | | `components/Choice/index.js` | `46 B` | | `components/Choice/script.js` | `5.8 kB` | | `components/Choice/styles.css` | `1.7 kB` | | `components/Container/index.js` | `46 B` | | `components/Container/script.js` | `4.5 kB` | | `components/Container/styles.css` | `1.2 kB` | | `components/Dialog/index.js` | `46 B` | | `components/Dialog/script.js` | `9.1 kB` | | `components/Dialog/styles.css` | `1.5 kB` | | `components/Divider/index.js` | `46 B` | | `components/Divider/script.js` | `1.7 kB` | | `components/Divider/styles.css` | `170 B` | | `components/Icon/index.js` | `46 B` | | `components/Icon/script.js` | `2.7 kB` | | `components/Icon/styles.css` | `153 B` | | `components/Image/index.js` | `46 B` | | `components/Image/script.js` | `6.7 kB` | | `components/Image/styles.css` | `724 B` | | `components/ImageUploader/index.js` | `46 B` | | `components/ImageUploader/script.js` | `11.7 kB` | | `components/ImageUploader/styles.css` | `2.4 kB` | | `components/Input/index.js` | `46 B` | | `components/Input/script.js` | `4.3 kB` | | `components/Input/styles.css` | `2.7 kB` | | `components/Link/index.js` | `46 B` | | `components/Link/script.js` | `3.1 kB` | | `components/Link/styles.css` | `220 B` | | `components/Loading/index.js` | `46 B` | | `components/Loading/script.js` | `2.2 kB` | | `components/Loading/styles.css` | `1.2 kB` | | `components/Menu/index.js` | `46 B` | | `components/Menu/script.js` | `11.0 kB` | | `components/Menu/styles.css` | `4.1 kB` | | `components/Modal/index.js` | `46 B` | | `components/Modal/styles.css` | `1.1 kB` | | `components/Notice/index.js` | `46 B` | | `components/Notice/script.js` | `4.6 kB` | | `components/Notice/styles.css` | `1.1 kB` | | `components/Pill/index.js` | `46 B` | | `components/Pill/script.js` | `2.8 kB` | | `components/Pill/styles.css` | `370 B` | | `components/PinInput/index.js` | `46 B` | | `components/PinInput/script.js` | `6.3 kB` | | `components/PinInput/styles.css` | `1.5 kB` | | `components/Popover/index.js` | `46 B` | | `components/Popover/script.js` | `10.9 kB` | | `components/Popover/styles.css` | `518 B` | | `components/ProgressBar/index.js` | `46 B` | | `components/ProgressBar/script.js` | `3.5 kB` | | `components/ProgressBar/styles.css` | `1.2 kB` | | `components/ProgressCircle/index.js` | `46 B` | | `components/ProgressCircle/script.js` | `3.9 kB` | | `components/ProgressCircle/styles.css` | `760 B` | | `components/Radio/index.js` | `46 B` | | `components/Radio/script.js` | `3.6 kB` | | `components/Radio/styles.css` | `1.5 kB` | | `components/SegmentedControl/index.js` | `46 B` | | `components/SegmentedControl/script.js` | `3.2 kB` | | `components/SegmentedControl/styles.css` | `1.4 kB` | | `components/Select/index.js` | `46 B` | | `components/Select/script.js` | `5.3 kB` | | `components/Select/styles.css` | `3.6 kB` | | `components/Skeleton/index.js` | `46 B` | | `components/Skeleton/script.js` | `4.3 kB` | | `components/Skeleton/styles.css` | `978 B` | | `components/StarRating/index.js` | `46 B` | | `components/StarRating/script.js` | `6.2 kB` | | `components/StarRating/styles.css` | `322 B` | | `components/Stepper/index.js` | `46 B` | | `components/Stepper/script.js` | `5.7 kB` | | `components/Stepper/styles.css` | `1.2 kB` | | `components/Text/index.js` | `46 B` | | `components/Text/script.js` | `4.6 kB` | | `components/Text/styles.css` | `4.7 kB` | | `components/Textarea/index.js` | `46 B` | | `components/Textarea/script.js` | `3.6 kB` | | `components/Textarea/styles.css` | `1.9 kB` | | `components/TextButton/index.js` | `46 B` | | `components/TextButton/script.js` | `3.9 kB` | | `components/TextButton/styles.css` | `2.1 kB` | | `components/Theme/index.js` | `46 B` | | `components/Theme/script.js` | `16.1 kB` | | `components/Theme/styles.css` | `264 B` | | `components/Toast/index.js` | `46 B` | | `components/Toast/script.js` | `9.1 kB` | | `components/Toast/styles.css` | `2.1 kB` | | `components/Toggle/index.js` | `46 B` | | `components/Toggle/script.js` | `3.5 kB` | | `components/Toggle/styles.css` | `2.7 kB` | | `components/VerticalDivider/index.js` | `46 B` | | `components/VerticalDivider/script.js` | `1.7 kB` | | `components/VerticalDivider/styles.css` | `239 B` | | [`LICENSE`](https://github.com/square/maker/blob/close-all-modals/LICENSE) | `552 B` | | [`package.json`](https://github.com/square/maker/blob/close-all-modals/package.json) | `5.3 kB` | | [`README.md`](https://github.com/square/maker/blob/close-all-modals/README.md) | `357 B` | | `utils/assert.js` | `1.0 kB` | | `utils/BlockFormControlLayout/index.js` | `46 B` | | `utils/BlockFormControlLayout/script.js` | `1.8 kB` | | `utils/BlockFormControlLayout/styles.css` | `234 B` | | `utils/constants.js` | `689 B` | | `utils/css-validator.js` | `933 B` | | `utils/debug.js` | `984 B` | | `utils/get-contrast.js` | `1.4 kB` | | `utils/InlineFormControlLayout/index.js` | `46 B` | | `utils/InlineFormControlLayout/script.js` | `2.5 kB` | | `utils/InlineFormControlLayout/styles.css` | `315 B` | | `utils/maker-colors.js` | `3.8 kB` | | `utils/RenderFn.js` | `766 B` | | `utils/Row/index.js` | `46 B` | | `utils/Row/script.js` | `2.5 kB` | | `utils/Row/styles.css` | `610 B` | | `utils/TouchCapture/index.js` | `25 B` | | `utils/TouchCapture/script.js` | `3.5 kB` | | `utils/Transition/index.js` | `25 B` | | `utils/Transition/script.js` | `2.4 kB` | | `utils/TransitionCollapse/index.js` | `25 B` | | `utils/TransitionCollapse/script.js` | `3.0 kB` | | `utils/TransitionFadeIn/index.js` | `25 B` | | `utils/TransitionFadeIn/script.js` | `2.3 kB` | | `utils/TransitionResize/index.js` | `25 B` | | `utils/TransitionResize/script.js` | `3.6 kB` | | `utils/TransitionResponsive/index.js` | `25 B` | | `utils/TransitionResponsive/script.js` | `2.2 kB` | | `utils/transitions.js` | `4.2 kB` | | `utils/TransitionSpringLeft/index.js` | `25 B` | | `utils/TransitionSpringLeft/script.js` | `2.3 kB` | | `utils/TransitionSpringUp/index.js` | `25 B` | | `utils/TransitionSpringUp/script.js` | `2.3 kB` | | `utils/TransitionStack/index.js` | `25 B` | | `utils/TransitionStack/script.js` | `2.3 kB` | | `utils/TransitionStaggered/index.js` | `25 B` | | `utils/TransitionStaggered/script.js` | `2.5 kB` |
Hidden files | File | Before | After | | ---------------------------------------------- | --------: | -----------------------: | | `components/Accordion/script.js.map` | `21.3 kB` | `21.3 kB` | | `components/Accordion/styles.css.map` | `5.9 kB` | `5.9 kB` | | `components/ActionBar/script.js.map` | `55.9 kB` | `55.9 kB` | | `components/ActionBar/styles.css.map` | `18.9 kB` | `18.9 kB` | | `components/Badge/script.js.map` | `21.6 kB` | `21.6 kB` | | `components/Badge/styles.css.map` | `5.2 kB` | `5.2 kB` | | `components/Blade/script.js.map` | `26.7 kB` | `26.7 kB` | | `components/Blade/styles.css.map` | `5.0 kB` | `5.0 kB` | | `components/Button/script.js.map` | `32.5 kB` | `32.5 kB` | | `components/Button/styles.css.map` | `16.2 kB` | `16.2 kB` | | `components/Calendar/script.js.map` | `28.8 kB` | `28.8 kB` | | `components/Calendar/styles.css.map` | `10.1 kB` | `10.1 kB` | | `components/Card/script.js.map` | `11.8 kB` | `11.8 kB` | | `components/Card/styles.css.map` | `1.6 kB` | `1.6 kB` | | `components/Checkbox/script.js.map` | `18.6 kB` | `18.6 kB` | | `components/Checkbox/styles.css.map` | `3.7 kB` | `3.7 kB` | | `components/Choice/script.js.map` | `25.4 kB` | `25.4 kB` | | `components/Choice/styles.css.map` | `7.9 kB` | `7.9 kB` | | `components/Container/script.js.map` | `18.1 kB` | `18.1 kB` | | `components/Container/styles.css.map` | `5.0 kB` | `5.0 kB` | | `components/Dialog/script.js.map` | `33.1 kB` | `33.1 kB` | | `components/Dialog/styles.css.map` | `9.3 kB` | `9.3 kB` | | `components/Divider/script.js.map` | `8.8 kB` | `8.8 kB` | | `components/Divider/styles.css.map` | `708 B` | `708 B` | | `components/Icon/script.js.map` | `12.6 kB` | `12.6 kB` | | `components/Icon/styles.css.map` | `1.6 kB` | `1.6 kB` | | `components/Image/script.js.map` | `23.4 kB` | `23.4 kB` | | `components/Image/styles.css.map` | `5.4 kB` | `5.4 kB` | | `components/ImageUploader/script.js.map` | `46.0 kB` | `46.0 kB` | | `components/ImageUploader/styles.css.map` | `20.2 kB` | `20.2 kB` | | `components/Input/script.js.map` | `21.1 kB` | `21.1 kB` | | `components/Input/styles.css.map` | `5.7 kB` | `5.7 kB` | | `components/Link/script.js.map` | `14.2 kB` | `14.2 kB` | | `components/Link/styles.css.map` | `3.4 kB` | `3.4 kB` | | `components/Loading/script.js.map` | `11.0 kB` | `11.0 kB` | | `components/Loading/styles.css.map` | `2.3 kB` | `2.3 kB` | | `components/Menu/script.js.map` | `42.1 kB` | `42.1 kB` | | `components/Menu/styles.css.map` | `13.8 kB` | `13.8 kB` | | `components/Modal/script.js.map` | `35.7 kB` | 16%↑`41.3 kB` | | `components/Modal/styles.css.map` | `11.0 kB` | 50%↑`16.4 kB` | | `components/Notice/script.js.map` | `18.4 kB` | `18.4 kB` | | `components/Notice/styles.css.map` | `5.1 kB` | `5.1 kB` | | `components/Pill/script.js.map` | `13.3 kB` | `13.3 kB` | | `components/Pill/styles.css.map` | `2.3 kB` | `2.3 kB` | | `components/PinInput/script.js.map` | `24.4 kB` | `24.4 kB` | | `components/PinInput/styles.css.map` | `7.9 kB` | `7.9 kB` | | `components/Popover/script.js.map` | `38.7 kB` | `38.7 kB` | | `components/Popover/styles.css.map` | `6.9 kB` | `6.9 kB` | | `components/ProgressBar/script.js.map` | `15.6 kB` | `15.6 kB` | | `components/ProgressBar/styles.css.map` | `3.5 kB` | `3.5 kB` | | `components/ProgressCircle/script.js.map` | `17.2 kB` | `17.2 kB` | | `components/ProgressCircle/styles.css.map` | `4.1 kB` | `4.1 kB` | | `components/Radio/script.js.map` | `17.5 kB` | `17.5 kB` | | `components/Radio/styles.css.map` | `3.6 kB` | `3.6 kB` | | `components/SegmentedControl/script.js.map` | `14.8 kB` | `14.8 kB` | | `components/SegmentedControl/styles.css.map` | `3.9 kB` | `3.9 kB` | | `components/Select/script.js.map` | `24.0 kB` | `24.0 kB` | | `components/Select/styles.css.map` | `6.6 kB` | `6.6 kB` | | `components/Skeleton/script.js.map` | `17.8 kB` | `17.8 kB` | | `components/Skeleton/styles.css.map` | `3.0 kB` | `3.0 kB` | | `components/StarRating/script.js.map` | `23.6 kB` | `23.6 kB` | | `components/StarRating/styles.css.map` | `6.8 kB` | `6.8 kB` | | `components/Stepper/script.js.map` | `22.2 kB` | `22.2 kB` | | `components/Stepper/styles.css.map` | `6.5 kB` | `6.5 kB` | | `components/Text/script.js.map` | `22.3 kB` | `22.3 kB` | | `components/Text/styles.css.map` | `11.5 kB` | `11.5 kB` | | `components/Textarea/script.js.map` | `17.9 kB` | `17.9 kB` | | `components/Textarea/styles.css.map` | `4.1 kB` | `4.1 kB` | | `components/TextButton/script.js.map` | `18.1 kB` | `18.1 kB` | | `components/TextButton/styles.css.map` | `5.2 kB` | `5.2 kB` | | `components/Theme/script.js.map` | `50.4 kB` | `50.4 kB` | | `components/Theme/styles.css.map` | `3.8 kB` | `3.8 kB` | | `components/Toast/script.js.map` | `38.7 kB` | `38.7 kB` | | `components/Toast/styles.css.map` | `14.7 kB` | `14.7 kB` | | `components/Toggle/script.js.map` | `18.3 kB` | `18.3 kB` | | `components/Toggle/styles.css.map` | `4.5 kB` | `4.5 kB` | | `components/VerticalDivider/script.js.map` | `8.9 kB` | `8.9 kB` | | `components/VerticalDivider/styles.css.map` | `760 B` | `760 B` | | `utils/assert.js.map` | `3.9 kB` | `3.9 kB` | | `utils/BlockFormControlLayout/script.js.map` | `8.4 kB` | `8.4 kB` | | `utils/BlockFormControlLayout/styles.css.map` | `762 B` | `762 B` | | `utils/constants.js.map` | `2.5 kB` | `2.5 kB` | | `utils/css-validator.js.map` | `3.3 kB` | `3.3 kB` | | `utils/debug.js.map` | `3.4 kB` | `3.4 kB` | | `utils/get-contrast.js.map` | `5.7 kB` | `5.7 kB` | | `utils/InlineFormControlLayout/script.js.map` | `12.6 kB` | `12.6 kB` | | `utils/InlineFormControlLayout/styles.css.map` | `1.4 kB` | `1.4 kB` | | `utils/maker-colors.js.map` | `14.8 kB` | `14.8 kB` | | `utils/RenderFn.js.map` | `2.8 kB` | `2.8 kB` | | `utils/Row/script.js.map` | `11.7 kB` | `11.7 kB` | | `utils/Row/styles.css.map` | `2.0 kB` | `2.0 kB` | | `utils/TouchCapture/script.js.map` | `12.0 kB` | `12.0 kB` | | `utils/Transition/script.js.map` | `10.7 kB` | `10.7 kB` | | `utils/TransitionCollapse/script.js.map` | `13.4 kB` | `13.4 kB` | | `utils/TransitionFadeIn/script.js.map` | `10.4 kB` | `10.4 kB` | | `utils/TransitionResize/script.js.map` | `14.4 kB` | `14.4 kB` | | `utils/TransitionResponsive/script.js.map` | `10.3 kB` | `10.3 kB` | | `utils/transitions.js.map` | `16.9 kB` | `16.9 kB` | | `utils/TransitionSpringLeft/script.js.map` | `10.5 kB` | `10.5 kB` | | `utils/TransitionSpringUp/script.js.map` | `10.5 kB` | `10.5 kB` | | `utils/TransitionStack/script.js.map` | `10.2 kB` | `10.2 kB` | | `utils/TransitionStaggered/script.js.map` | `11.0 kB` | `11.0 kB` |

πŸ€– This report was automatically generated by pkg-size-action

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 15.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: