-
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
Use state for dimensions visualizers #33560
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
6cbfc20
Use state for spacing visualizers
ajlende 792ec91
Merge branch 'trunk' into fix/spacing-visualizer
ajlende ba74b2e
Merge branch 'trunk' into fix/spacing-visualizer
ajlende b38dcfa
Rename variables for clarity
ajlende 81adb49
Finish rename
ajlende a626a8e
Fix nested values overwriting
ajlende 0c2b487
Rename visualizer props as __experimental
ajlende b93408f
Merge branch 'trunk' into fix/spacing-visualizer
ajlende File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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.
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.
Could we just use
__unstableMarkNextChangeAsNotPersistent
here and__unstableMarkLastChangeAsPersistent
before thesetAttributes
at theonChange
function? The second action is because of this: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/store/reducer.js#L497There 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 wasn't aware of these, but after skimming through the code, if I understand it correctly, it looks like they are basically just a complicated way to toggle between triggering the
onInput
versusonChange
props for theBlockEditor
. It doesn't really solve the root-cause of this issue—usingattributes
to store editor-specific data.Using
attributes
for anything other than properties needed to render the final block is what I think is the hacky part. As long as we're usingattributes
for editor data, there are always going to be opportunities for bugs like this one to crop up.The ideal case would be incorporating
BoxControlVisualizer
directly into the hook so only the block.json needs to be updated for the support. Maybe someone has an idea for how to do that in the overhaul that @aaronrobertshaw mentioned. The changes for blocks using the hook are pretty minimal with this approach, but you're right, they aren't nothing.The implementation would be simpler for hooks that don't have nested properties. In that simple case, you could just pass
setVisualizers
fromuseState
directly to__experimentalSetVisualizers
, and that could be documented withBoxControlVisualizer
.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.
Exactly!
Also I wasn't able to reproduce adding extra props in
style
besides thepadding ones
. Do I have to do something specific?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.
Do you mean
attributes.style
? If so, #31839, has a screen recording and the steps to reproduce the issue.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 might not be so straightforward when the differences between blocks (and their markup) are taken into account. More complicated again, when both padding and margin wish to be visualized.
With the current visualizer, displaying margins is particularly problematic and can require adding a wrapping element which creates a disjoint between the markup in the editor and the frontend.
One previous suggestion on how the visualizer could work was making it not rely on a parent element and instead maybe use a ref/popover.
For the moment, I'd think being able to handle the visualization on a block by block basis would be best.