Spacer: Use layout context to work out whether or not Spacer should be horizontal in the Row block #39743
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
andrewserong
added
[Type] Enhancement
andrewserong
force-pushed
the
update/spacer-to-be-horizontal-in-row-block
branch
from
c990522 to
ca5c57f
Compare
|
Size Change: +125 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
andrewserong
added
[Type] Bug
|
It may or may not be an issue, but if you add a Spacer to a Row block it works as expected, but if you then transform it to a Group the Spacer seems to disappear as the height is set to 0px: spacer.mp4 |
|
Oh, good catch @glendaviesnz, thanks for testing! I wonder what the expected behaviour would be here. Do you think something like if the orientation is switched, then we convert the current width value to height? I wonder if users would be more likely to expect the value to be converted, or for it to be effectively removed via the |
I would be inclined to try this for starters |
Thanks, if I don't get a chance today, I'll have a play with that tomorrow! |
|
I remember @tellthemachines worked on a similar problem in the Navigation block, so will probably be interested in this. |
There was a problem hiding this comment.
Thanks for addressing this one @andrewserong 👍
It tests pretty well for me although I did encounter the same minor issue @glendaviesnz noted.
Along those same lines, after having set a spacer's width or height in one context and then dragging or grouping it into a new context, it surprised me a bit the retention of the "unused" value.
I'm not sure what the better behaviour is but it could be something to consider while addressing the zero height issue raised previously.
Screen.Recording.2022-03-28.at.6.25.01.pm.mp4
apeatling
approved these changes
Mar 28, 2022
There was a problem hiding this comment.
Other than the items mentioned above, this tested as expected for me. I was able to insert a space in the row block and adjust it horizontally. Inserting a space between two paragraph blocks continued to work vertically.
One slight annoyance is that the [+] button often obscures the handle for resizing a horizontal spacer.
ntsekouras
reviewed
Mar 29, 2022
| @@ -17,6 +17,10 @@ | |||
| "enum": [ "all", "insert", false ] | |||
| } | |||
| }, | |||
| "providesContext": { | |||
| "layout": "layout", | |||
| "orientation": "orientation" | |||
There was a problem hiding this comment.
The block doesn't have an orientation attribute. Also I'm wondering if this is something we should consider adding in the layout support by default and not adhoc in Group block. Do you know other similar use cases?
There was a problem hiding this comment.
Thanks! I've had a go in 3ac8f3c at moving the providesContext to the layout block support instead. I think a good case could be made for passing it down as context in all cases where the layout support is opted into, particularly given that child blocks still need to explicitly opt-in to allow accessing that context.
andrewserong
force-pushed
the
update/spacer-to-be-horizontal-in-row-block
branch
from
ca5c57f to
3ac8f3c
Compare
andrewserong
requested review from
TimothyBJacobs,
spacedmonkey and
ellatrix
as code owners
|
Just dusting this PR off: I've just converted this back to Draft while I explore the idea of passing the Layout context down at the block support level, instead of having to have blocks like Group explicitly provide the context in I haven't tested this with Native yet (I think I'll need to do some updates there), so I'll continue chipping away tomorrow. |
…ntal when a default orientation value is not available
andrewserong
force-pushed
the
update/spacer-to-be-horizontal-in-row-block
branch
from
3ac8f3c to
b69a81d
Compare
|
Update: I'm about to close my laptop and go AFK, so just writing a little update of where this PR is at in case anyone would like to pick it up. Here's the status:
So, it might be better to defer the React Native side of things to a separate PR that also coincides with adding proper support for the Row variation in native. Note: if anyone else is struggling to run React Native locally on an M1 laptop, I could get it working if I first follow the steps in the React Native docs for M1 laptops, and then update the Anyway, hopefully some of that info will help anyone else who might like to look at this — if anyone wants to have a go at finishing off this PR, feel free to jump in! Otherwise, I'm happy to pick it up again when I'm back in a few weeks. Thanks! 👋 |
|
This PR is quite stale now, and since there is other work that will be explored for widths in the row block which also relates to layout context at the child-of-layout-block level (#44467), I'll close this PR out for now. |
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
Update the Spacer block to look at
layoutdata to infer orientation if no default orientation is available. This primarily addresses an issue when the Spacer block is a child of the Row block.Why?
The Row block does not set a
horizontalorientation explicitly — its orientation is implicit.How?
layoutattribute to the context that's provided for blocks that opt-in to the layout supportTesting Instructions
Screenshots or screencast