Migrate the Post 'Switch to draft' button to `ConfirmDialog` component #37491

chad1008 · 2021-12-17T15:51:34Z

Related: #34153

Description

This PR aims to migrate the post editor's Switch to draft button away from the current confirm() implementation and instead use the new experimental ConfirmDialog component.

How has this been tested?

Running WordPress 5.8.2 via wp-env:

Create a new post
Add some content, a title, etc.
Publish the post
Click the Switch to draft button
Monitor console for any unexpected errors
Confirm that the new ConfirmDialog component is rendered instead of the previous browser-based confirmation triggered by confirm()
Click Cancel, and confirm that the post remains published
Click Switch to draft again, and this time hit OK. Confirm the post is reverted to a draft

I've tested in the latest Chrome, Firefox, and Safari.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

chad1008 · 2021-12-17T23:58:41Z

Marking as draft - there's a bug that causes the Switch to draft button to be disabled if the user clicks Cancel the first time the confirmation is displayed - I can fix it by lifting the showConfirmDialog state to a parent component. I'll reopen after making that adjustment.

chad1008 · 2021-12-20T18:17:33Z

Fixed - state is now handled more effectively, and the confirmation dialog is reset after the cancel button is clicked.

chad1008 · 2022-01-07T18:09:50Z

Updated this PR to simplify the implementation of ConfirmDialog (cc @ciampo)

ciampo

Hey @chad1008 , thank you for working on this!

Overall the changes test well according to your instructions! I left a couple of minor comments, but this is definitely looking good!

One more questions — I'm not expert about the editor package, but it may be good to double check if this could affect any unit/e2e tests (although I can't see any failures), or if this interaction would benefit from adding an e2e tests at all (not sure who's better-suited to answer that).

packages/editor/src/components/post-switch-to-draft-button/index.js

ciampo · 2022-01-21T09:21:54Z

Update:

I marked all of the memoization-related conversations as resolved for now, as discussed in this comment
there's still an open comment that needs addressing
not sure if you managed to look into fixing any broken e2e/unit tests (if any) and potentially adding any new ones

Even with the last comment addressed, I'd still be reluctant to merge this PR until we find a good fix for the z-index issue (see #37959).

ciampo · 2022-02-04T15:14:32Z

With #37959 merged, we can consider this PR unblocked.

@chad1008 , feel free to continue working on this PR (I believe we're only missing any potential e2e test work) and ping me when it's ready for the next round of review!

chad1008 · 2022-02-08T22:19:22Z

e2e tests added! I didn't see any existing tests for the switch to draft button so I created new ones. The new tests should cover:

confirming and canceling the confirmation
posts and pages
published vs scheduled
large and small viewports

...working out to 16 tests in total, all passing consistent locally.

ciampo

Excellent job, @chad1008 ! 🎉

The tests are great — I just wonder if we can write them in a way that tests less the implementation details (classnames, dom structure...) and more the semantics and the way a user would interact with the page (e.g. click a button with a certain text).

I left 2 inline comments specifically to @wordpress/components classnames — it'd be great if we could refactor the tests.

When we're done with that, we'll be able to merge this PR too!

packages/e2e-tests/specs/editor/various/switch-to-draft.test.js

chad1008 · 2022-02-10T11:13:58Z

Thanks for the feedback @ciampo, I'll take a look!

This is helpful for me on a more general level... these are both sections of the test I was able to "borrow" from other tests I've worked on recently (i.e. "I need to schedule this post, OH, I know where someone else did that, I'll just reuse that flow!").

After some of your feedback on other PRs, I think I've gotten better about relying on DOM structure when writing my own XPaths/selectors, but I clearly need to apply that same philosophy to code snippets I'm re-using!

ciampo · 2022-02-10T13:19:14Z

these are both sections of the test I was able to "borrow" from other tests I've worked on recently
...
After some of your feedback on other PRs, I think I've gotten better about relying on DOM structure when writing my own XPaths/selectors, but I clearly need to apply that same philosophy to code snippets I'm re-using!

Absolutely! It'd be also great if, once you find a good way to write the e22e tests on this PR, you open another PR to improve the tests you "borrowed" from :)

This blog post partially explains the reasoning behind not to test the implementation details (it's focused on React unit testing, but some core principles are the same)

chad1008 · 2022-02-10T20:57:10Z

I've updated the two class-based XPaths mentioned above, and made a couple other small improvements - there are now no selectors reliant on DOM structure in the test!

ciampo

Thank you @chad1008 !

Before merging, it's probably better to double-check that the approach we're taking (ie. avoiding testing implementation details like dom structure, and writing tests more from the point of view of the user) — just cc'ing a few folks who have recently worked in the e2e-tests package in case they any potential issues cc @aaronrobertshaw @andrewserong @ramonjd @Mamaduka @jorgefilipecosta

ciampo · 2022-02-11T10:13:26Z

packages/e2e-tests/specs/editor/various/switch-to-draft.test.js

@@ -137,6 +135,11 @@ describe( 'Clicking "Switch to draft" on a published post/page', () => {
 					expect( postStatus ).toBe( 'publish' );
 				} );
 				it( `should revert a published ${ postType } to a draft if confirmed`, async () => {
+					// Capitalize postType for use in snackbar XPath
+					const capitalizedPostType =


Idea: is there a way in Gutenberg (eg. a utility function) to match text in a case-insensitive way?

In case there isn't, in order to make the test more resilient to capitalisation changes, we could instead:

convert the expected text toLowerCase()

convert the text to be found in the DOM to lower case too, by using the translate function

I haven't tried this approach myself, but it could (should) work!

We might also be able to use the lower-case() or matches() functions within the XPath selector.

Those are both new to me, thanks for the links @aaronrobertshaw! I wasn't able to implement either, even when testing directly in Chrome's dev tools with strings I know should match it didn't recognize the elements I was targeting. Not sure if it's user error on my part (likely) or something else, but for now I've gone with @ciampo's toLowerCase() suggestion.

I don't love everything about this implementation... for one thing we're locating the snackbar notification by finding just the word "published" or "reverted" respectively, then pulling the text, lower casing it, and comparing it a full string to my current post type. Feels kind of like taking the long way around, and almost makes me wonder if I shouldn't just stop at identifying a snackbar notif with published/reverted inside it 🤔

Not sure if it's user error on my part (likely) or something else,

Maybe it's related to the version of XPath (it seems like those functions are available from version 2.0)?

I don't love everything about this implementation... for one thing we're locating the snackbar notification by finding just the word "published" or "reverted" respectively, then pulling the text, lower casing it, and comparing it a full string to my current post type. Feels kind of like taking the long way around, and almost makes me wonder if I shouldn't just stop at identifying a snackbar notif with published/reverted inside it 🤔

It does seem convoluted, but I don't know if there's a better way to test for a confirmation that the post was published / reverted. Let's keep it as-is for now, and iterate on it in case we find a better solution!

Mamaduka · 2022-02-11T10:37:36Z

I just merged the #38524. I will try to have a look on e2e tests later today.

aaronrobertshaw · 2022-02-14T03:41:48Z

Before merging, it's probably better to double-check that the approach we're taking (ie. avoiding testing implementation details like dom structure, and writing tests more from the point of view of the user) — just cc'ing a few folks who have recently worked in the e2e-tests package in case they any potential issues cc @aaronrobertshaw

In general, I'd agree if we can avoid relying on the DOM structure and write them from the user's point of view, we should.

In practice, for a lot of e2e tests, I'm not sure how feasible that is. It appears to be pretty common practice in the existing tests to target classes etc. Just because that's the way it was done doesn't mean we should continue that way.

I haven't had a chance to properly test this but I did leave one suggestion. If we leverage the matches() XPath function we could possibly use its case-insensitive flag to avoid having to find and manipulate text capitalization.

…ConfirmDialog` component

…og` cancellation

…text

ciampo

LGTM 🚀

Thank you @chad1008 for iterating on this and adding e2e tests!

chad1008 force-pushed the migrate/switch-to-draft-confirm-dialog branch from a989f3d to c370f94 Compare December 17, 2021 16:04

chad1008 marked this pull request as draft December 17, 2021 23:55

chad1008 marked this pull request as ready for review December 20, 2021 18:19

chad1008 force-pushed the migrate/switch-to-draft-confirm-dialog branch from fc850ec to 014ba4b Compare December 20, 2021 19:56

skorasaurus added [Feature] Component System WordPress component system [Package] Components /packages/components labels Dec 29, 2021

ciampo self-requested a review January 3, 2022 16:17

ciampo assigned chad1008 Jan 3, 2022

ciampo requested a review from mirka January 3, 2022 16:20

chad1008 force-pushed the migrate/switch-to-draft-confirm-dialog branch from 014ba4b to 2066a3a Compare January 7, 2022 18:04

ciampo requested a review from fullofcaffeine January 9, 2022 16:14

ciampo reviewed Jan 9, 2022

View reviewed changes

packages/editor/src/components/post-switch-to-draft-button/index.js Show resolved Hide resolved

packages/editor/src/components/post-switch-to-draft-button/index.js Show resolved Hide resolved

packages/editor/src/components/post-switch-to-draft-button/index.js Outdated Show resolved Hide resolved

This was referenced Jan 9, 2022

Migrate Edit Navigation screen "Delete menu" button from confirm() … #37492

Merged

Migrate Post Template Delete button from confirm() to ConfirmDialog #37535

Merged

chad1008 mentioned this pull request Jan 11, 2022

Migrate post privacy confirmation from confirm() to ConfirmDialog #37602

Merged

7 tasks

chad1008 requested review from ajitbohra, nerrad and ntwb as code owners February 8, 2022 22:16

ciampo reviewed Feb 10, 2022

View reviewed changes

packages/e2e-tests/specs/editor/various/switch-to-draft.test.js Outdated Show resolved Hide resolved

packages/e2e-tests/specs/editor/various/switch-to-draft.test.js Outdated Show resolved Hide resolved

chad1008 force-pushed the migrate/switch-to-draft-confirm-dialog branch from 47dd1bb to 0fd2339 Compare February 10, 2022 20:55

chad1008 mentioned this pull request Feb 10, 2022

Post Visibility e2e test: Improve XPath selector to avoid reliance on DOM structure #38717

Merged

8 tasks

ciampo reviewed Feb 11, 2022

View reviewed changes

chad1008 added 7 commits February 14, 2022 14:44

migrate the post switch to draft button from confirm() to the new `…

66f8659

…ConfirmDialog` component

add handler method to reset showConfirmDialog state on `ConfirmDial…

1754134

…og` cancellation

simplify ConfirmDialog state using the built in isOpen prop

b39ec2c

explicitly close ConfirmDialog after confirmation

1867258

add switch to draft e2e tests for published posts and pages

0473fb0

add switch to draft e2e tests for scheduled posts and pages

0919773

Improve XPath selectors to avoid reliance on DOM structure

cce7fbb

chad1008 force-pushed the migrate/switch-to-draft-confirm-dialog branch from 0fd2339 to cce7fbb Compare February 14, 2022 19:44

use toLowerCase() for more resiliant case matching of notification …

d6a2014

…text

ciampo self-requested a review February 16, 2022 19:23

ciampo approved these changes Feb 16, 2022

View reviewed changes

ciampo merged commit 368f854 into WordPress:trunk Feb 16, 2022

github-actions bot added this to the Gutenberg 12.7 milestone Feb 16, 2022

chad1008 mentioned this pull request Feb 21, 2022

Improve switch to draft button e2e test robustness #38971

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the Post 'Switch to draft' button to `ConfirmDialog` component #37491

Migrate the Post 'Switch to draft' button to `ConfirmDialog` component #37491

chad1008 commented Dec 17, 2021 •

edited

Loading

chad1008 commented Dec 17, 2021

chad1008 commented Dec 20, 2021

chad1008 commented Jan 7, 2022

ciampo left a comment

ciampo commented Jan 21, 2022 •

edited

Loading

ciampo commented Feb 4, 2022 •

edited

Loading

chad1008 commented Feb 8, 2022 •

edited

Loading

ciampo left a comment

chad1008 commented Feb 10, 2022

ciampo commented Feb 10, 2022

chad1008 commented Feb 10, 2022

ciampo left a comment

ciampo Feb 11, 2022

aaronrobertshaw Feb 14, 2022

chad1008 Feb 14, 2022

ciampo Feb 16, 2022

Mamaduka commented Feb 11, 2022 •

edited

Loading

aaronrobertshaw commented Feb 14, 2022

ciampo left a comment

Migrate the Post 'Switch to draft' button to ConfirmDialog component #37491

Migrate the Post 'Switch to draft' button to ConfirmDialog component #37491

Conversation

chad1008 commented Dec 17, 2021 • edited Loading

Description

How has this been tested?

Checklist:

chad1008 commented Dec 17, 2021

chad1008 commented Dec 20, 2021

chad1008 commented Jan 7, 2022

ciampo left a comment

Choose a reason for hiding this comment

ciampo commented Jan 21, 2022 • edited Loading

ciampo commented Feb 4, 2022 • edited Loading

chad1008 commented Feb 8, 2022 • edited Loading

ciampo left a comment

Choose a reason for hiding this comment

chad1008 commented Feb 10, 2022

ciampo commented Feb 10, 2022

chad1008 commented Feb 10, 2022

ciampo left a comment

Choose a reason for hiding this comment

ciampo Feb 11, 2022

Choose a reason for hiding this comment

aaronrobertshaw Feb 14, 2022

Choose a reason for hiding this comment

chad1008 Feb 14, 2022

Choose a reason for hiding this comment

ciampo Feb 16, 2022

Choose a reason for hiding this comment

Mamaduka commented Feb 11, 2022 • edited Loading

aaronrobertshaw commented Feb 14, 2022

ciampo left a comment

Choose a reason for hiding this comment

Migrate the Post 'Switch to draft' button to `ConfirmDialog` component #37491

Migrate the Post 'Switch to draft' button to `ConfirmDialog` component #37491

chad1008 commented Dec 17, 2021 •

edited

Loading

ciampo commented Jan 21, 2022 •

edited

Loading

ciampo commented Feb 4, 2022 •

edited

Loading

chad1008 commented Feb 8, 2022 •

edited

Loading

Mamaduka commented Feb 11, 2022 •

edited

Loading