-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Editor: Add 'isDeletingPost' selector #44012
Conversation
Size Change: +374 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
It looks good but as a rule I'm a little wary of adding APIs that aren't used anywhere. It's best when API design is informed by real world use. Can we include a fix for #15755? |
@noisysocks, added in 3302c8c. Testing Instructions
ScreencastCleanShot.2022-09-12.at.10.57.03.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic looks good to me. I recommend one more review on this, I am still a bit new to redux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
It might be nice to update the styles so that .is-disabled.is-busy
has red text. Right now the text turns blue.
Thanks for the reviews 🙇
That would a good general Button improvement. I will push it as a follow-up. |
@noisysocks, the "blue" is the hover color for the button. Now I'm not sure if we want to change that. |
Sure but it should match the hover colour for a destructive button that doesn't have |
@noisysocks, the hover color is correct. The blue color is coming from the gutenberg/packages/components/src/button/style.scss Lines 184 to 186 in e5dca9d
P.S. I don't mind creating a follow-up PR, but I would need help/confirmation from the design team 😅 |
Ah, alright then let's leave it 👍 |
What?
Resolves #43863
Fixes #15755.
PR introduces a new
isDeletingPost
selector for the editor store to check if the current post is being deleted.Why?
There's a long-standing issue where users on slow networks can click the "Move to trash" button multiple times, which results in failure notices. See #15755.
To fix this, the editor needs a way to signal the components that the post is being deleted.
Initially, I tried to refactor the
trashPost
action to usedeleteEntityRecord
, so we could useisDeletingEntityRecord
. But some of the logic in the editor relies ongetCurrentPost
to be available even when a post is trashed, anddeleteEntityRecord
also removes records from the store.A good example of this will be the
BrowserURL
component, which handles page redirection when the post status is changed totrash
. However, there probably are more cases that I've not found yet.How?
The trash post now dispatches two new actions -
REQUEST_POST_DELETE_START
andREQUEST_POST_DELETE_FINISH
.Testing Instructions
Unit tests are passing.