-
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
Update: Block top level useSetting paths. #37428
Update: Block top level useSetting paths. #37428
Conversation
Size Change: +65 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
@@ -15,6 +15,8 @@ import { __EXPERIMENTAL_PATHS_WITH_MERGE as PATHS_WITH_MERGE } from '@wordpress/ | |||
import { useBlockEditContext } from '../block-edit'; | |||
import { store as blockEditorStore } from '../../store'; | |||
|
|||
const blockedPaths = [ 'color', 'typography', 'spacing' ]; |
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.
My first instinct would be to also block border
and layout
for consistency. However, layout
is the only one that is actually accessed directly.
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.
I left useSetting( 'layout' )
as it seems the use cases make sense. I removed the border.
@@ -104,6 +106,9 @@ export default function useSetting( path ) { | |||
|
|||
const setting = useSelect( | |||
( select ) => { | |||
if ( blockedPaths.includes( path ) ) { | |||
return 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.
Can we throw a message (and add a comment) so the purpose is clear?
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.
The direction makes sense to me and it's the minimal paths we need to cover. I've left a couple of comments to gather your thought (but didn't want to block this if you judge them as something not to do).
3b7e066
to
11f5019
Compare
11f5019
to
9fb2886
Compare
Fixes: #37094
This PR blocks top-level useSetting calls e.g: useSetting( 'color' ), in order to avoid bugs given that these calls don't take back-compatibility and other logic into consideration.