-
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
Site and Widgets Editor: Fix complementary area not opening #28732
Conversation
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.
It seems like WordPress has this policy to avoid refactorings for a good reason 😅
https://make.wordpress.org/core/2011/03/23/code-refactoring/
Thank you for catching it! In this case, though, we have good reasons to refactor usage since it will bring benefits soon.
Size Change: +6 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
@@ -78,7 +78,7 @@ function Editor() { | |||
isFullscreenActive: isFeatureActive( 'fullscreenMode' ), | |||
sidebarIsOpened: !! select( | |||
interfaceStore | |||
).getActiveComplementaryArea( editSiteStore ), | |||
).getActiveComplementaryArea( 'core/edit-site' ), |
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.
editSiteStore.name
?
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.
Or how about hacking getActiveComplementaryArea
to check for objects with name
property? 😛 So we can avoid these magical strings.
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.
gutenberg/packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Lines 43 to 45 in fd24f06
let sidebar = select( interfaceStore ).getActiveComplementaryArea( | |
editPostStore.name | |
); |
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.
This is how it's done somewhere else in the codebase. I'd still think about making getActiveComplementaryArea
work with store objects.
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.
You can check if it is an object and have name
inside getActiveComplementaryArea
. The only question is if that should be based on the store name really. It's hard to tell.
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.
IMHO there are no expectations for it to be a store name, so we shouldn't accept a store object as scope
.
A string is totally fine as it is, and passing store.name
is perfectly reasonable to me.
One thing I'd point out, though, is that at this point we are (almost everywhere) using the store.name
for the selector, but we are still creating the <ComplementaryArea>
components with an hardcoded string as scope.
E.g.
scope="core/edit-post" |
It's not a big deal, but we might want to update them all with store.name
instead, to keep them consistent, or risk breaking them in the off chance a store changes name.
Apologies for the push after the approvals, but I discovered that the same issue was happening in the Widgets Editor as well. |
Description
In #28695 we replaced the hardcoded
core/edit-site
strings to indicate a Redux store with the actual store, in an ongoing refactor.Among the various correct replacements, a string that was actually supposed to be
core/edit-site
was replaced as well.This string indicated the scope of
getActiveComplementaryArea
, acore/interface
selector used to check the status of the right-side sidebar (block, document, global styles, plugins, etc.).Passing the
core/edit-site
store instead of thecore/edit-site
string prevented the function to target the correct sidebars.How has this been tested?
Tested in the Site Editor and Widgets Editor.
(Note that typically FSE themes don't register sidebars, and therefore don't have the Widgets Editor, and vice versa).
Just make sure that the right-side sidebar can be toggled, by trying the block inspector, document settings, global styles, plugins, you name it.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: