Components: Add experimental ConfirmDialog
#34153
Conversation
|
Size Change: +177 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
fullofcaffeine
force-pushed
the
replace/confirm-et-al
branch
5 times, most recently
from
fd6a6a5
to
48fafb3
Compare
fullofcaffeine
force-pushed
the
replace/confirm-et-al
branch
from
48fafb3
to
72cd96b
Compare
fullofcaffeine
force-pushed
the
replace/confirm-et-al
branch
from
5bc14bb
to
f4a3396
Compare
fullofcaffeine
force-pushed
the
replace/confirm-et-al
branch
from
840ece6
to
d5e8d73
Compare
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?
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 |
fullofcaffeine
force-pushed
the
replace/confirm-et-al
branch
from
392ebfa
to
90ceb3e
Compare
|
I've changed the implementation to use the
|
| @@ -67,7 +67,7 @@ describe( 'Confirm', () => { | |||
| expect( onConfirm ).toHaveBeenCalled(); | |||
| } ); | |||
|
|
|||
| 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 () => { | |||
| @@ -73,13 +73,16 @@ function ConfirmDialog( | |||
| [ handleEvent, onConfirm ] | |||
| ); | |||
|
|
|||
| const cancelLabel = __( 'Cancel' ); | |||
| const confirmLabel = __( 'OK' ); | |||
There was a problem hiding this comment.
Alright:
- Modal changes have been applied in a separate PR (
Modal: add__experimentalHideHeaderprop, wrap inforwardref#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.