Layout child fixed size should not be fixed by default and should always have a value set #46139
Conversation
tellthemachines
requested review from
spacedmonkey and
ellatrix
as code owners
tellthemachines
added
[Feature] Layout
|
Size Change: +3.04 kB (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is testing nicely for me, and I like the idea of adding the toggle control 👍
Just left a comment about the setAttributes call within useEffect, since I think I remember a similar thing causing performance issues with the Group block in the past, but I can't quite remember why it did, I'm sorry! 😅
| const { selfStretch, flexSize } = childLayout; | ||
| const { selfStretch, flexSize, breakOutOfLayout = false } = childLayout; | ||
|
|
||
| useEffect( () => { |
There was a problem hiding this comment.
I remember we needed to remove the useEffect in the Group block back in #44176. Will adding a useEffect here have similar performance concerns as the earlier Group block useEffect?
I'm wondering if we need to change the attribute via setAttribute or if it'd be possible to use a different temporary variable for determining which control to display, if it's more about which tab is shown when the block is initially selected, rather than changing the attributes for the block 🤔
There was a problem hiding this comment.
Offhand I'd think not because this control only loads once the block is selected. I think the perf issues we were seeing with using an effect in the Group edit function were related to the editor content taking longer to load.
In any case, I'm happy to change if there's a better approach! I played around with it a bit but couldn't find another way to tell if the component is rendering for the first time, or just updating as a result of a change of value.
There was a problem hiding this comment.
Offhand I'd think not because this control only loads once the block is selected.
Ah, good point, yes, the rendering is neatly tucked away inside the DimensionsPanel isn't it — sounds like we can go with this approach for now and revisit if need be 👍
lib/block-supports/layout.php
Outdated
| @@ -334,10 +334,11 @@ function gutenberg_render_layout_support_flag( $block_content, $block ) { | |||
| $child_layout_styles = array(); | |||
|
|
|||
| if ( 'fixed' === $block['attrs']['style']['layout']['selfStretch'] && isset( $block['attrs']['style']['layout']['flexSize'] ) ) { | |||
| $has_shrink = isset( $block['attrs']['style']['layout']['breakOutOfLayout'] ) && $block['attrs']['style']['layout']['breakOutOfLayout']; | |||
There was a problem hiding this comment.
Tiny style nit (don't worry about updating this if you prefer the explicit approach):
Would it be worth using _wp_array_get here with a default value of false?
E.g. something like _wp_array_get( $block, array( 'attrs', 'style', 'layout', 'breakOutOfLayout' ), false )
|
Thank you for this! It's nice to see this being mainly a "flex-shrink" toggle! I'd love for us to start with flex-shrink enabled by default, and add the toggle separately. I can sense you're concerned that we won't add such a toggle — I think we should, but I also think it needs the right prominence and placement, and a separate control like this probably isn't the right one. The thing is, we have this same challenge for fluid typography, this one:
Those 2.25rem in the screenshot are clamp-based fluid, despite being custom and explicit. It would be good for design to think of a universal interface for making these values non-fluid or non-responsive. One basic idea is to have this option live in the unit selector: Another is an explicit toggle that's integrated into the component-row itself, like "link/unlink". But it needs a little more design work, and it would be good to not hold up the flex-shrink change based on the toggle design. What do you think? |
|
Thanks for working on this, I agree with everything Joen said re the toggle. As for the original issue, it still feels a bit buggy that you can select a fixed width and save with the empty input. Could we auto-populate the input with the inherent value instead? For example here:
The calculated with is 135px, so when I click 'Fixed' in the width control, the input would automatically be populated with 135px. This would also help folks understand exactly what the 'Fixed' option means. |
|
I took a stab at where a toggle might live in #46128 (comment) |
There was a problem hiding this comment.
This is still working well for me after the removal of the toggle, and defaults to no shrink value being applied 👍
The calculated with is 135px, so when I click 'Fixed' in the width control, the input would automatically be populated with 135px. This would also help folks understand exactly what the 'Fixed' option means.
That's a cool idea seeing if we can grab the inherent width and pre-populate it here. It can be challenging to introspect the width of elements reliably from within block controls, though, so I think that sounds like a good thing to look at in a follow-up after this PR lands.
I've tried doing this in the latest commit. How well it works depends on how many child blocks there are as well as their intrinsic sizes: how the flex container distributes values changes depending on whether the element has an explicit size or not. Weirdly enough, if the block's siblings are able to grow further, setting an explicit value equal to the block's current size can actually cause it to shrink:
|
Oh whoops, just saw this now. That might have been best, but I just went and did it 😂 |
| @@ -96,7 +108,7 @@ export function ChildLayoutEdit( { | |||
| }, | |||
| } ); | |||
| } } | |||
| value={ flexSize } | |||
| value={ flexSize || blockSize } | |||
There was a problem hiding this comment.
Using the value as a fallback in all cases appears to cause an odd behaviour when editing / removing the fixed value altogether, because it winds up getting a closer to fresh value for the width. Here's a screengrab (initially set to 200px, then after deleting the value and clicking out of the field, it sets it to 95px:
Would it work to use blockSize within the onChange handler when switching between fit and fixed instead? Edit: ah wait, I see you're already doing that. I guess the question is then, do we need to include the fallback on value?
There was a problem hiding this comment.
I'm not sure I understand the question. If we remove the fallback from value the field won't be pre-populated, which I think is a requirement?
There was a problem hiding this comment.
If we remove the fallback from value the field won't be pre-populated, which I think is a requirement?
Ah, good point, my apologies, I missed that detail of the requirement that James mentioned. I'd been thinking of the Fit -> Fixed selection and not the saved empty Fixed state problem 🤔
There was a problem hiding this comment.
No worries, always worth asking!
There was a problem hiding this comment.
Just thought I'd hack around a little bit. The only other way I could come up with addressing that requirement while preventing the 95px issue from showing up, would be to add a bit of logic so that when the block is initially selected, it displays as if fit were selected if fixed is selected but flexSize is empty:
const [ showFit, setShowFit ] = useState();
useEffect( () => {
if ( selfStretch === 'fixed' && ! flexSize ) {
setShowFit( true );
}
}, [] );Then, in the ToggleGroupControl change handler we'd call setShowFit( false ) whenever the value changes. And the bits where we're referring to selfStretch would become the slightly awkward:
value={ showFit ? 'fit' : selfStretch || 'fit' }
help={ helpText( showFit ? 'fit' : selfStretch ) }
With that in place, I think we could then remove the blockSize fallback in value={ flexSize || blockSize }, but overall it would be slightly different behaviour than what was asked for.
Feel free to ignore all that, though, I was mostly curious to better understand how it's working! 🙂
There was a problem hiding this comment.
Wouldn't that hide the input altogether when it has no value? I'm not sure we want that; the idea is that if 'fixed' is selected the input should be pre-populated with the current width/height of the block, so we have to pass blockSize as the value unless there's an existing flexSize.
There was a problem hiding this comment.
Wouldn't that hide the input altogether when it has no value?
Yes, it would hide it when initially selected if it doesn't have a flexSize value, and would default to display 'fit' instead. I'm not really advocating for that, but it was the only behaviour I could figure out that avoided accidentally displaying the wrong width value when a user clears out the Fixed input field and then tabs away.
There was a problem hiding this comment.
Another thought: the issue of the wrong width value only appears when you tab away from the cleared input field, and is correct again if you select another block and then click to re-select the block you were initially working on. Personally, I'd be fine with the idea of merging this PR as-is, and us continue to fix/tweak in follow-ups if need be.
No worries! I wasn't sure how hard it'd be to retrieve the value. Good work digging in so quickly! |
Could this be a rounding issue? Going back to my screenshot in #46139 (comment), notice that the paragraph is actually Could we force values like this to round up to the next pixel? IE 138.00001 becomes 139. That might solve the shrinking problem? |
|
Thanks for re-testing @jameskoster!
It doesn't seem to be a result of rounding, from hacking around locally to get it to round up. It looks like it's to do with what happens when we set an explicit
So, I think displaying a value unfortunately highlights the gap between the user entered value and the fuzziness of how As a workaround, one way to potentially deal with the jumpiness when toggling between Fit and Fixed would be to display the pixel value in the input field, but not actually set it on the block until a user changes the value. However, as soon as they go to change the value, they'd still see the jumpiness. What do you think? |
|
It seems okay that there's a little shift, even if it's curious. Can it be related to the box model? I.e. is it accurate if there's no padding? |
|
I agree a little shift is ok, but perhaps its best to force an upwards shift so that we eliminate the extreme text reflow we see in #46139 (comment)? I guess adding a couple of pixels to the calculated value would solve it? |
|
Thanks again for taking a look.
It doesn't appear to be, because we're applying
I did a bit more digging, and the short answer appears to be that no, a couple of pixels won't solve it. I was curious to learn more about why that's the case, so there's a longer explanation below, hope y'all don't mind me sharing the longer version! 🙂 It's a really interesting one, but the issue appears to be with how browsers deal with In the below screenshot we have a Row block where the left two items are set to "Fit". In this case, we will set the left most block to "Fixed" and use its default value based on the calculation in this PR. Note the reflow, with the value being set to In order to get the width of the block to line up to roughly the same width of the original row, we need to set an explicit
The reason for this seems to be touched on in the MDN entry for flex-basis:
Examining the block after applying the
So, where does this leave us? I think it means that if we remove Based on this, in my view, I think I would probably lean toward abandoning attempting to pre-fill the |
|
Since @tellthemachines isn't around today, and just in case it's desirable to get the |
|
Thanks for the digging! Perhaps I am misinterpreting, but it seems like a pretty big problem that setting a width of 200px in the settings doesn't actually result in a block that is 200px wide? |
|
Thanks for the detailed description of the problem @andrewserong ! The short of it is that, due to the way |
This reverts commit 2ec5ac7.
This is always going to happen if we don't set |
…ays have a value set (WordPress#46139) * Change behaviour of fixed child size to not be fixed by default * Fix boo-boo * Remove unshrink control * Determine fixed size automatically * Revert "Determine fixed size automatically" This reverts commit 2ec5ac7.
Why?
Fixes #46116 and addresses this discussion
Makes fixed flex child size shrinkable by default, with optional toggle to un-shrink it. Also makes sure that a value for 'fixed' setting exists.
How?
flex-shrinkshould only be0if the block is explicitly set to unshrinkable.Testing Instructions
500pxin the input, save;Screenshots or screencast