Components: Add experimental ConfirmDialog#34153
Conversation
|
Size Change: +177 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
fd6a6a5 to
48fafb3
Compare
48fafb3 to
72cd96b
Compare
5bc14bb to
f4a3396
Compare
840ece6 to
d5e8d73
Compare
jsnajdr
left a comment
There was a problem hiding this comment.
The way how the react-confirm library works is that when confirm is called, it creates a new container div in document.body, renders the dialog there as a completely independent React tree, and after the confirmation is done, removes the UI including the container and resolves the promise.
One of the consequences is that the useContextSystem hook will see an empty ComponentContext, not the context for the rest of the app. It's the same with any other context provider. Is that OK for us?
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this, @fullofcaffeine !
On top of a few inline comments, here's the rest of my initial feedback:
Accessibility
I haven't done a full accessibility audit, but I already noticed that this component is currently not handling keyboard focus.
I guess this is one more reason for considering using an existing component for the Modal dialog functionality.
Splitting in multiple PRs
Ideally, a PR that focuses on a component should only include code strictly related to the component.
In this case, it would be better if the changes in packages/editor/src/components/post-visibility/index.js (and any other similar changes) would be applied in a separate PR.
I'm fine with keeping those code changes for now (for ease of testing/review), but they should be removed before merging
Finally, it'd be great to have @diegohaz also take a look at this PR.
Yeah, I'm aware of that. I don't fully understand what the On the plus side, this allows us to keep the API very close to the native EDIT: Also, see my reply here: #34153 (comment). EDIT2: The |
392ebfa to
90ceb3e
Compare
|
I've changed the implementation to use the
|
Agreed 👍🏻 . I used this as an additional test case to see how it worked in the editor context, but I'll extract this into a separate PR soon. |
There was a problem hiding this comment.
Thank you @fullofcaffeine for addressing the massive amount of feedback received so far! This PR is in excellent state, and so to speed up the process, I've taken the freedom to push a few very minor tweaks.
Regarding the changes that add the showTitle prop to the Modal component, my preference would be to move them to a separate PR in order to keep this PR focused on the ConfirmDialog component — I will take care of that.
This means that the plan now looks like this:
I will open a separate PR with the changes toDone, opened #36831ModalI will remove the changes toDone. This PR is expected to have some build errors while at this stage, until we get to step 4Modalfrom this PRThe separate PR aboutDoneModalchanges gets reviewed and merged- We rebase this PR, add CHANGELOG notes, give a final sanity check, and merge 🚀
- You can open smaller follow up PRs to iterate on this component
If that sounds, I will go ahead and start to execute the plan.
| // parent component. In that case, `selfClose` might do more harm than | ||
| // good, so we disable it. | ||
| const isIsOpenSet = typeof isOpenProp !== 'undefined'; | ||
| setIsOpen( isIsOpenSet ? isOpenProp : true ); |
There was a problem hiding this comment.
I wonder what happens if:
isOpenPropis set tofalse— the dialog is not shown- later,
isOpenPropis unset — would the dialog suddenly show?
Definitely somewhat of an edge case, can be tackled in a follow-up PR
There was a problem hiding this comment.
Hmm, not sure. I can add a test case for that in a follow-up PR, I'll take note!
| component: ConfirmDialog, | ||
| title: 'Components (Experimental)/ConfirmDialog', | ||
| parameters: { | ||
| knobs: { disabled: false }, |
There was a problem hiding this comment.
We should move away from knobs and rewrite this story to use controls instead — perfect task for a follow-up PR !
There was a problem hiding this comment.
Sounds good! Note taken!
That sounds like a good plan! 👍🏻 |
| } ); | ||
|
|
||
| it( 'should not render if closed by clicking `Cancel`, and the `onConfirm` callback should be called', async () => { | ||
| it( 'should not render if closed by clicking `Cancel`, and the `onCancel` callback should be called', async () => { |
| ); | ||
|
|
||
| const cancelLabel = __( 'Cancel' ); | ||
| const confirmLabel = __( 'OK' ); |
ciampo
left a comment
There was a problem hiding this comment.
Alright:
- Modal changes have been applied in a separate PR (#36831)
- This PR has been updated to incorporate these changes (and a CHANGELOG entry)
@fullofcaffeine and @jasmussen, this PR is ready to be merged in my opinion — it would be great if you could also give it a final sanity check
|
What's a good way to test this? In a quick checkout, it appears that changing the post visibility invokes the classic ok/cancel confirm dialog. If you have a screenshot of the dialog, we can check the metrics are right. |
Yes, you're correct — in the end, this PR only introduces the new Because of that, we could also work on any improvements/tweaks in follow-up PRs (as far as they are not blockers)
Sure, the best way to test the new component is in Storybook. Here's what it currently looks like: confirm-dialog.mp4 |
|
Nice, thanks for the video. I think that looks good, definitely good enough to land. I suspect there are some margins and tweaks we can follow up on, looks like there's some trailing uneven spacing between paragraph and button row that might be solved by a stacking layout, but nothing that need block this. 👍 👍 |
Thank you! @fullofcaffeine , could give it a final look as well? Feel free to merge in case everything looks good on your end :) Regarding next steps, I guess they could be:
With each of these points to be carried out in separate issues/PRs as a follow-up. What do you think, Marcelo? |
Just to clarify, this could potentially be some complexity that's handled by components themselves in a nice way. I was inspired by this stacked box principle, for example. |
Sounds good! Thank you for all the help and good vibes here, it was lots of fun working on this! |
Description
Requires #36831 to be merged first — some build errors are expected until that PR gets merged, and this PR gets updated to include those changes
Add the experimental
ConfirmDialogcomponent.The goal is to use this component to replace the usage of the native sync
confirmthat is bound to be deprecated at some point (see related issues for more info).How has this been tested?
Automated tests and storybook
ConfirmDialogcomponent, and verify stories.Change post visibility to private
This is here to test the confirm in a real-world scenario, and will be removed before this PR is merged.
Screen[cast,shot]
Peek.2021-10-07.18-57.mp4
I misspelled "privately" in the screencast above as "privetely". This has been fixed in the code.
Types of changes
New feature
TODO
READMEfor more details). Is it worth keeping it? Are we even going to use it in practice considering we'll probably use a wrapper singleton component later (to be implemented in a follow-up PR) at the app top-level to implement the Confirm in the editor?xicon, Cancel, pressing Escape, or clicking the overlay will close the dialog AND call theonCancelcallback. I think this is correct UX-wise, except that we might not want to close/cancel upon clicking the overlay, as one could argue this is fragile as the user could easily misclick and close + cancel by accident. I do think that closing has a 1:1 relationship with canceling, though, so that if the dialog is closed this effectively means not confirming which means canceling.Checklist:
*.native.jsfiles for terms that need renaming or removal).PostVisibilitycomponent. They are there only to exemplify a real-world usage of the component.Related to: #33937 and #16583.