[Pattern Overrides] Use a single checkbox to turn on pattern overrides for all allowed attributes #57009
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevin940726
added
[Feature] Synced Patterns
kevin940726
changed the title
|
Size Change: -626 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f6b59db. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7258753482
|
talldan
reviewed
Dec 15, 2023
talldan
reviewed
Dec 15, 2023
talldan
reviewed
Dec 19, 2023
Comment on lines
77
to
89
| <CheckboxControl | ||
| __nextHasNoMarginBottom | ||
| label={ __( 'Allow instance overrides' ) } | ||
| checked={ Object.keys( syncedAttributes ).some( | ||
| ( attributeName ) => | ||
| attributes.connections?.attributes?.[ | ||
| attributeName | ||
| ]?.source === 'pattern_attributes' | ||
| ) } | ||
| onChange={ ( isChecked ) => { | ||
| updateConnections( isChecked ); | ||
| } } | ||
| /> |
There was a problem hiding this comment.
I think would also be good to disable the checkbox when the block's only connectable attributes are connected to something else (e.g. a custom field).
At the moment, if you try checking it nothing happens.
There was a problem hiding this comment.
I wonder if we should just completely avoid rendering it if that happens. Disabling it might confuse the users too as we don't show any other helper text. Completely hiding it has the same issue but I think it's easier 😅. Given that both features (meta binding and pattern overrides) are not stable yet, I think it's best to keep it simple here for now. WDYT?
There was a problem hiding this comment.
Yeah, I wasn't sure, but I'm good with either 👍
I'll make sure to add it to the tracking issue as something to reconsider.
49 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
As opposed to the current behavior of turning on syncing each attribute for a given block, use a single checkbox to turn on pattern overrides for all allowed attributes (a hardcoded allowed list).
Why?
While per attribute override and locking is a compelling feature, it also surfaces the concept of
attributesto the end users, which should remain an implementation detail. Until we can decide how the user flow would be for individual attribute override, maintaining an allowed list for the supported attributes and turn them all on at once seems like a better option at this stage.How?
Some tiny refactoring in the pattern syncing controls.
The wording can be changed though!
Testing Instructions
Screenshots or screencast