Fix input not showing when switching to "Fixed" width #56660
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
|
Size Change: +741 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2d929a5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7040046292
|
tellthemachines
added
[Type] Regression
There was a problem hiding this comment.
Thanks for picking this up!
Unfortunately this only appears to resolve the issue when toggling from Fit to Fixed directly. If I go from Fill to Fixed, it still doesn't reveal the unit control for me (presumably because there hasn't been a change to force an update when switching between Fill and Fixed?):
2023-11-30.13.49.54.mp4
|
Thanks for testing @andrewserong ! I've pushed an update to remove the checks entirely and always render |
There was a problem hiding this comment.
That's working nicely in the UI for me, now! It means we remove some of the optimisations from #55762, but I think fixing the UI is probably a priority for now. The only potential blocker I encountered is that it seems this removes the disableLayoutStyles check that prevents the styling rules and classname from being output. With this PR applied I can see the flex-grow: 1 rule is output for a block set to fill, whereas on trunk running a theme with add_theme_support( 'disable-layout-styles' ); they get skipped. Here's the output for this PR:
Comment on lines
-479
to
-480
| return ! select( blockEditorStore ).getSettings() | ||
| .disableLayoutStyles; |
There was a problem hiding this comment.
Will this removal mean the styles and classnames get generated and output if disableLayoutStyles is true?
There was a problem hiding this comment.
Oh, of course, well spotted! Let me see if this still works if we keep it.
There was a problem hiding this comment.
Ok I think I've fixed it now 😅
andrewserong
approved these changes
Nov 30, 2023
There was a problem hiding this comment.
Nice one, thanks for the back and forth!
✅ Switching between any of the toggles to Fixed or back now reliably shows the unit control
✅ If a theme has add_theme_support( 'disable-layout-styles' ); active, then the CSS rules and classname are skipped as expected, while the toggling between controls still works
✅ It looks safe to me that blocks are still run through BlockWithChildLayoutStyles as useStyleOverride will bail and not call setStyleOverride if css is an empty string, and the classname will not be applied 👍
LGTM! ✨
|
As usual, @andrewserong is too fast! The screencast doesn't show it, but the input shows also when toggling between Fixed, Fit and Fill, and then back to Fixed. 2023-11-30.15.40.35.mp4🚢 |
|
Thanks for the reviews folks! I'm going to go ahead and merge this now, and I'll open a new issue for resetting the toggle. I've just checked and that issue is in 6.4 so it predates this one. |
|
Thank you, @tellthemachines! |
|
I've created #56667 to address the toggle issue. |
derekblank
pushed a commit
that referenced
this pull request
Dec 7, 2023
* Fix input not showing when switching to "Fixed" width * Always render BlockWithChildLayoutStyles * Render BlockWithChildLayoutStyles if layout is enabled
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 the regression described here. The refactor of the layout rendering logic indirectly caused a bug in the child layout controls component, as detailed here.
This fix makes sure that the check for rendering child layout styles insidewithChildLayoutStylesis consistent with the logic inBlockWithChildLayoutStyles, so that component will only render when there is CSS to output.Update: this fix removes the check inside
withChildLayoutStylesaltogether so thatBlockWithChildLayoutStylesis always rendered, and it decides whether there are styles to render or not.Testing Instructions
TODO: the "Fixed" toggle should reset to "Fit" on page reload if there's no actual value set.
Edit: the toggle reset issue is described here and might need a separate fix.
Testing Instructions for Keyboard
Screenshots or screencast