-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search: Make custom widths work with float alignments #31919
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: -981 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
I'll let someone review the code here, but I can confirm that this fixes the issue described in #31517. Looks good in both the front and back end: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working well for me, nice work. It'd be great to get this merged so I can use it, and some of the utility functions with #31719
Nice work @aaronrobertshaw. This is testing well for me in tt1-blocks and a few other themes (e.g. Maywood). I noticed some themes set their own
The main issue I ran into is that for some reason, in Twenty Twenty One (the non-FSE version), within the editor it doesn't appear to be working for me (not sure if this is to do with my dev environment?): On the front end of the site, it's rendering correctly, though. For example, this is at 75%: |
Thanks for the thorough testing @andrewserong, it's very much appreciated! 👍
I'll take a closer look at the TwentyTwentyOne (non block based theme). There might be some extra wrinkles depending on layout support for the theme. Regardless, we'll need to get it working there as well before this can land. |
@andrewserong It looks like the issue you had with TwentyTwentyOne is another theme specific issue. It has a media query style targeting floated elements within the editor, setting a max-width on them. I'm not sure we can differentiate between that max width in the computed styles and the one provided by the custom layout for say the group block when it's nested. I also couldn't see any sweet spot to add a new CSS rule in with greater specificity than the TwentyTwentyOne style but less than the one generated by a group block's custom layout when in a block-based theme. The reason that the 100% width is the only option that appears to change the size of the floated search block, is that there is a min-width enforced on the search block which is larger than the 75% and lower options given the max-width the theme sets on left/right aligned blocks. I'm tempted to suggest we proceed with this and have themes update if they want to allow more than the fixed widths they currently set. Doing so would unblock the work being done adding the ability to expand/collapse the search block from its button. |
Thanks for getting to the bottom of that @aaronrobertshaw!
I agree, personally, I think it makes sense not to try to work around themes that are already applying a max width, and let them opt-in by updating their styling if they'd like to 👍 |
Only fixes the width issue for floated search block on the frontend. Editor still wraps block with .wp-block[data-align=right] element that needs to be handled separately.
b7aa4d8
to
8acf8c1
Compare
I've added a |
This is a tricky one @aaronrobertshaw! Adding in the max-width as an inline style does make it work more nicely in Hemingway, but it looks like it has a side effect in Twenty Twenty One (not the blocks version). In that theme, the max-width used prior to this change is a little unusual: It's using a With the inline override, because it doesn't have the margin subtracted from it, it causes the search block to be much wider than we'd expect: In this case, I feel like the theme is doing something a bit non-standard, but maybe it's safer not to override it? |
Thanks for catching that. Perhaps my version of TwentyTwentyOne was out of date, I don't see the styles you highlighted. I've rolled back the change adding the max-width. As you say it's probably best not to try and predict all the ways theme styles might break it. |
Sounds good, thanks for updating that. For what it's worth, I was testing with Twenty Twenty One v1.3 downloaded from the themes directory (rather than from the Github repo). |
Fixes: #31517
Related: #31812
Description
PoC for alternate approach to applying widths to search blocks which are aligned left/right (using floated wrapper element).
Possible Next Steps
How has this been tested?
Test Block Code
Screenshots
Types of changes
Bug fix
Checklist:
*.native.js
files for terms that need renaming or removal).