Prepublish Panel: Disable the Publish and Cancel buttons while saving #32889
Conversation
|
Size Change: -2.83 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
Here is an associated issue: |
|
I think I found what needs to be really changed, it's the way the |
… not have been saved yet
d7df47b. This was originally added by #18029 (which enabled multi-entity saving for the first time), and I think that it was done to enable the 'Publish' button when there were, well, non-post entities to save (even if there weren't any post related changes). However, this also caused the present (problematic) behavior. Let's see if this has any unwanted ripple effects (e.g. breaks tests in |
|
Okay, I think this is ready for another round of review now 😄 |
My first thought looking at this from the e2e side - would the
But considering this, it sounds like disabling the 'Publish' button would make sense. |
TBH, The change to the button state logic ended up pretty small -- I'm really just removing the
That's actually the problem that prompted me to work on this. In that e2e test, we edit the Site Title block (and thus, the site title setting), save it, press 'Cancel', and navigate to the settings screen to verify that the changes took effect. Since the 'Cancel' button isn't properly disabled, we press it too early and navigate away from the page when saving the site title hasn't actually finished. (We could work around this by looking at other indicators rather than the 'Cancel' button's disabled state, such as the loading spinner in the Prepublish panel -- see screenshots in the PR desc -- but I'd argue that the buttons really should reflect the same state as that loading spinner. FWIW, their event handlers already do -- clicking the 'Cancel' button is a no-op while we're saving.) |
There was a problem hiding this comment.
the previous behavior was plain wrong, so I'm fairly confident that this is the "right" fix.
I tend to agree with you there, that previous && ! hasNonPostEntityChanges logic seems the opposite of what it should have been.. it really doesn't make sense the way it was.
unit test coverage in packages/editor/src/components/post-publish-button/test/index.js seemed fairly exhaustive.
I basically hope that if this ends up breaking something else, it'll be so catastrophic that we'll fix it quickly (and add test coverage to prevent further regressions)
Agreed!
|
e2e test failures are unrelated and have been fixed in |
Well, that happened: #33072. (See #33072 (comment) for more details.) I'll revert. |
|
Oof! Tricky business in these panels/buttons. At least we pointed out the test coverage we are missing 😆 . I should have thought to test something like a reusable block or template part, I was assuming any non-'current'-post entities would behave the same as the root site entity. |
|
Second attempt here (WIP): #33140 |
Description
Found while working on #32868; see the conversation starting at #32868 (comment) for more background.
Currently, when editing a post in such a way that a "multi-entity save" is required (i.e. updates to things that aren't just limited to the post content (and other post attributes), but e.g. site title and the like), the user is prompted with a panel to save those changes prior to publishing the post.
Once they're done saving, they're presented with the regular pre-publish panel, including the familiar 'Publish' and 'Cancel' buttons. Actually, those buttons are visible even before the rest of the pre-publish panel is: The panel might still show a spinner that indicates that entities are being saved, but the buttons are already present -- and clickable.
This has turned out to be a problem in e2e tests, where the headless browser is quick enough to press one of those buttons, even though entities haven't been saved yet. However, it's of course possible that a user might also be quick enough (e.g. if behind a slow network connection).
In order to ensure that entities are really saved/updated prior to allowing the user to click the 'Publish' or 'Cancel' button,
this PR makes themEdit: Nope, see #32889 (comment).disabledwhileisSavingistrue.Note that there are quite a number of conditions around
isSaving(and an extraforceIsSavingprop 😅 ) inPostPublishButton, so it'd be great if someone more familiar with those could review this PR 😄How has this been tested?
trunk, they will be enabled; on this PR, they will be disabled.trunk, try clicking 'Cancel' while the spinner is still there. Reload the page (and discard any changes), and observe that the site title hasn't been changed.Screenshots
Before:
After: