-
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
Global styles: leave screen if current shadow entry gets deleted #65935
Conversation
Size Change: +15 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
const updatedShadows = shadows.filter( ( s ) => s.slug !== slug ); | ||
setShadows( updatedShadows ); | ||
goTo( `/shadows` ); | ||
setShadows( shadows.filter( ( s ) => s.slug !== slug ) ); |
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.
No need to call goTo
anymore here, since deleting the shadow will cause the useEffect
added above to run, and currentShadow
will be undefined
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.
LGTM!
By the way, the issue this PR is trying to solve was already present in WP 6.6, so a backport isn't necessarily necessary.
I don't have a strong opinion, but I think a backport would be fine 👍
If this PR is rebased the tests should pass. See #65939 |
538994b
to
eb9a4aa
Compare
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.
Works well in my testing and the code looks good 👍
Just a minor stylistic suggestion, but it's totally optional.
🚀
packages/edit-site/src/components/global-styles/shadows-edit-panel.js
Outdated
Show resolved
Hide resolved
) * Global styles: leave screen if current shadow entry gets deleted * Refine approach * Apply feedback --- Co-authored-by: ciampo <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: nith53 <[email protected]> Co-authored-by: benniledl <[email protected]>
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 8a54551 |
…dPress#65935) * Global styles: leave screen if current shadow entry gets deleted * Refine approach * Apply feedback --- Co-authored-by: ciampo <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: nith53 <[email protected]> Co-authored-by: benniledl <[email protected]>
What?
Fixes #65930
This PR prevents the Global Styles "shadow edit" screen from crashing when a user resets the styles while on the screen and editing a custom shadow preset.
Why?
Bug fix
How?
The bug was caused by the fact that the user would be allowed to edit a stale shadow preset, after it had been removed from the global styles.
This PR checks for the current screen shadow to actually exist — and if it doesn't, it navigates back to the parent screen, where all up-to-date shadows are listed.
Testing Instructions
Make sure that the bug flagged in #65930 can't be replicated on this PR.
Screenshots or screencast
Kapture.2024-10-08.at.11.02.18.mp4