Get orientation for Spacer block from parent layout #49322
Merged
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
tellthemachines
added
[Block] Spacer
tellthemachines
requested review from
MaggieCabrera,
talldan,
getdave,
draganescu and
andrewserong
tellthemachines
changed the title
|
Size Change: +36 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
draganescu
approved these changes
Mar 24, 2023
Apologies, I was typing too quickly! I see you've retained the existing |
There was a problem hiding this comment.
This is generally working quite nicely! I ran into a couple of minor issues while testing with Row and Stack variations.
With Row block, it lists the height as null when dragging it. Should we use a 48px minHeight for consistency with the rule to help out the Stack variation?
When switching the parent Row between Row and Stack, because a height value isn't available yet, I found it hard to click on the Spacer block within the editor canvas. This mightn't be something to try fixing in this PR, though, if it seems more complex to deal with.
I'm not sure why this happens, because there's a default 24px height defined for horizontal orientation already. Fwiw, I can reproduce it on trunk with the Navigation block, as well as the other bug you mention above:
It might be worth looking into these separately as it doesn't seem they were caused by these changes. |
andrewserong
approved these changes
Mar 27, 2023
Yeah I thought that there might be 3rd party blocks providing that context, so better avoid breaking it 😅 |
tellthemachines
force-pushed
the
fix/spacer-orientation
branch
from
f6007cc
to
8a9fd87
Compare
tellthemachines
added
the
Needs Dev Note
24 tasks
Dev noteSpacer block gets orientation from parent block layoutSpacer blocks inside a |
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 #36197.
Prelude to addressing #38022.
Changes the Spacer orientation logic so that it looks for a value in the parent layout. Spacers inside Rows should now be oriented correctly.
Also removes the context provider from Navigation block because Spacer can now read from its layout instead.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast