Fix/flex controls bugs #47533
Merged
Fix/flex controls bugs #47533
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
[Type] Bug
|
Size Change: +97 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
Comment on lines
12
to
13
| // Stretch to button width if it is set. | ||
| width: 100%; |
There was a problem hiding this comment.
This is a bit of a nitpick, but I'm wondering if it'd be safer to move this to the block where we have rules for .wp-block-buttons > .wp-block-button so that it only applies to button blocks that are the direct child of the core Buttons block (around line 44 or so)? From memory, there can be some difficult cases either with super old markup from back when the Button block didn't need to be in a Buttons container, or with plugins that borrow / extend styles from the Button block, so I was wondering if that might help restrict the scope of where this gets applied 🤔... of course I very well might be overthinking this!
There was a problem hiding this comment.
I can't think of a situation where we wouldn't want the interactive element to occupy the full width of its wrapper, because having something styled as a button not being clickable in its whole area would be really crappy UX. But I'm not really sure why Button needed a wrapper in the first place so 🤷 happy to leave it alone for now.
I'd prefer to make a direct change like this instead of adding all sorts of specificity to it, as it seems like a sensible default, but maybe better do it earlier in the release cycle.
There was a problem hiding this comment.
I'd prefer to make a direct change like this instead of adding all sorts of specificity to it, as it seems like a sensible default, but maybe better do it earlier in the release cycle.
Totally agree, I think it sounds like a great default and I also prefer avoiding the extra specificity where we can. I'm mostly just cautious because of having previously merged block CSS changes I thought were safe and then found out later that I'd broken something edge-casey.
Since it'd arguably be fixing a bug, once this PR lands, I suppose we could open up a separate PR just for the Button CSS change and then ping for wider review from designers, etc? Since it'd be a bug fix we might have a chance of still landing it for 6.2 past the feature freeze cutoff
|
Flaky tests detected in bd9b95f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4040312869
|
There was a problem hiding this comment.
Thanks for the follow-up!
✅ Stretch option and icon in JustifyContentControl is working correctly
✅ Resets when switching orientation appear to work well now
❓ Left a question about where to place the button styles — I'm mostly thinking of legacy markup on older sites, or Classic themes that might override widths in unique ways. It can be difficult to predict the impact of changing core block styles sometimes 😅. I noticed also that height might need to be handled for the button block, too, as the stretch items option in the horizontal orientation otherwise looks like it doesn't do anything, since it applies to the button wrapper:
Some potential options might be:
- Remove the Button block CSS change from this PR and otherwise merge as-is since the other changes look stable and are non-controversial and/or:
- Make the rule for adding width or height to the Button block be more specific to the classnames for content justification stretch, so that we know it's only being applied if one of these new options is in use
What do you think?
This was referenced Mar 2, 2023
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?
Follow-up to #47134.
Why?
Fixes issues described in this comment.
How?
Testing Instructions
space-betweenjustification andstretchvertical alignment;stretchjustification andspace-betweenvertical alignment;Testing Instructions for Keyboard
Screenshots or screencast