Grid: Unset the rowStart and columnStart attributes when a block is removed from a manual layout #64186
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
talldan
added
[Type] Enhancement
|
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. |
talldan
commented
Aug 2, 2024
| updates[ clientId ] = update; | ||
| } | ||
| } | ||
| } ); | ||
| } else { | ||
| // When in auto mode, remove all of the columnStart and rowStart values. | ||
| for ( const clientId of blockOrder ) { |
There was a problem hiding this comment.
Something I noticed is that the code always loop through these clientids in auto mode, even if the layout didn't change (one of the other deps might have caused the effect to re-run).
A small optimization might be to also keep track of the previous value of gridLayout.isManualPlacement and only remove the attributes when previousIsManualPlacement is true.
I can make a follow-up if you think that's worth doing.
There was a problem hiding this comment.
I went ahead and made a PR - Grid perf: Avoid iterating auto grid inner blocks unless mode specifically changed
TBH though, and I mentioned it in the PR description, maybe unsetting the attributes should be handled in the onChange of the toggle component for the grid mode.
|
Size Change: +1.44 kB (+0.08%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
talldan
commented
Aug 2, 2024
Comment on lines
28
to
55
| function unsetGridPositionAttributes( | ||
| attributes, | ||
| unsetSpanAttributes = false | ||
| ) { | ||
| let layout; | ||
| let hasUpdate; | ||
|
|
||
| if ( unsetSpanAttributes ) { | ||
| const { columnStart, rowStart, columnSpan, rowSpan, ...restLayout } = | ||
| attributes.style?.layout ?? {}; | ||
| layout = restLayout; | ||
| hasUpdate = !! columnStart || rowStart || columnSpan || rowSpan; | ||
| } else { | ||
| const { columnStart, rowStart, ...restLayout } = | ||
| attributes.style?.layout ?? {}; | ||
| layout = restLayout; | ||
| hasUpdate = !! columnStart || rowStart; | ||
| } | ||
|
|
||
| if ( hasUpdate ) { | ||
| if ( ! Object.keys( layout ).length ) { | ||
| return { style: { ...attributes.style, layout: undefined } }; | ||
| } | ||
| return { | ||
| style: { ...attributes.style, layout }, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ugh, this started off simple, but got more complicated as things developed. I wonder if it's worth unDRYing it.
There was a problem hiding this comment.
Yeah I don't think there's a lot of value to having a helper function here given the need for unsetSpanAttributes.
If you're wanting to clean it up, maybe try using setImmutably to set the attributes to undefined instead of destructuring. That might read a little bit nicer. I'm pretty sure we can safely set attributes to undefined to remove them because serialize() will strip them out.
I also don't mind just having the destructuring inlined. It's a common enough pattern.
There was a problem hiding this comment.
Yep, I originally tried with setting them to undefined so it does work 👍
I'll try some refactorings.
There was a problem hiding this comment.
I've updated it to use setImmutably, which is ok, though it's not so great with multiple attributes.
One negative is that it leaves behind an empty layout object after everything has been unset inside it. I feel like it's adding too much complexity when I try to handle that, so maybe it's ok to leave it. 🤔
There was a problem hiding this comment.
Pushed one more update which I think is better, solves the empty layout object issue.
Still have the style object which can be empty, but at least that's more likely to be used outside of grid.
There was a problem hiding this comment.
Maybe we need unsetImmutably which handles removing dangling objects.
quiet voice I miss lodash...
|
Something I noticed this PR doesn't support at the moment is dragging from one grid block to another. That's a situation where we might want to retain the grid position and span attributes, but it introduces complexity due to each grid being a self-managed island. We could possibly do something like checking the removed block's parent, but then we'd have to also get the attributes of that parent and check it's a grid along with the layout type. Still, it might be nice to support. Thoughts? |
What happens when you do this? |
noisysocks
reviewed
Aug 5, 2024
Right now removes the span attributes because the block is outside the grid, so the block would reset to 1x1 when dropped. Compared to what's in trunk this does seem like it'd be a regression, so I think it'll need to be fixed. I'll push some more commits. |
|
Yeah gotcha. Skipping the attribute removal if the new parent is a grid sounds reasonable. We have plenty of |
…switching to auto
…leaving all attributes in place
talldan
force-pushed
the
update/grid-layout-attributes-for-removed-inner-blocks
branch
from
dcce092 to
8cf4899
Compare
|
This should be ready for re-review 👍 |
talldan
force-pushed
the
update/grid-layout-attributes-for-removed-inner-blocks
branch
from
94ff565 to
b102b37
Compare
|
Flaky tests detected in b102b37. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10245846943
|
noisysocks
approved these changes
Aug 6, 2024
packages/block-editor/src/components/grid/use-grid-layout-sync.js
Outdated
Show resolved
Hide resolved
talldan
deleted the
update/grid-layout-attributes-for-removed-inner-blocks
branch
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?
Fixes #63973
Unset the rowStart and columnStart attributes when a block is removed from a manual grid layout.
How?
Employs
usePreviousto keep track of the previousblockOrder, then looks for blocks that were there but aren't any longer in theuseGridLayoutSynceffect.Testing Instructions
Other things to try