Search block: use font size for search icon #59159
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
ramonjd
added
[Type] Bug
|
Size Change: +39 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ramonjd
requested review from
tellthemachines,
ellatrix and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
commented
Feb 19, 2024
ramonjd
commented
Feb 19, 2024
|
Flaky tests detected in ccfb9ff2d7ee0bc0dd52c7687eff37c2d5598704. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7998489411
|
ramonjd
force-pushed
the
add/search-block-icon-size
branch
from
619ac11
to
7b29216
Compare
There was a problem hiding this comment.
Nice work here! It's mostly working well for me, just ran into one XSS issue and left a comment.
One thing I'm wondering about is what to do about font-sizes set in global styles. Unfortunately I'm not sure if there's a neat way to handle it. Here's how it's looking in the site editor if you go to adjust the font size of all search blocks:
At the individual block level, though, it's working nicely:
I imagine for the PHP rendering, we might be able to do a look-up to see if there's a global styles value for the block, but I'm not sure if that's possible in the post editor. If only there were a way to set the width of the SVG icon based on what the font size for the current element is 🤔
I see the Navigation block uses getComputedStyle in one area of its edit component... would it be too hacky to do something like that for the JS-side of things?
|
Thanks for testing @andrewserong
Good question. I'm not sure either. I ignored it on purpose to validate this approach (using the font size as the icon dimensions) first.
The tricky bit is when the values are clamped. I suppose it would be possible to parse the max size from the clamp function in a similar fashion. I wouldn't be against it 😄 I'll chip away at this one - I don't think there's any rush. Cheers!! |
ramonjd
force-pushed
the
add/search-block-icon-size
branch
from
c212c43
to
9d8d9e0
Compare
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
ramonjd
commented
Feb 21, 2024
ramonjd
commented
Feb 21, 2024
ramonjd
force-pushed
the
add/search-block-icon-size
branch
2 times, most recently
from
265941f
to
ccfb9ff
Compare
ramonjd
commented
Feb 22, 2024
| @@ -12,6 +12,8 @@ $button-spacing-y: math.div($grid-unit-15, 2); // 6px | |||
| svg { | |||
| min-width: $grid-unit-30; | |||
| min-height: $grid-unit-30; | |||
| width: 1.25em; | |||
| height: 1.25em; | |||
There was a problem hiding this comment.
🏆 to @andrewserong for suggesting this.
2024-02-22.13.15.30.mp4
andrewserong
approved these changes
Feb 22, 2024
There was a problem hiding this comment.
Nice one, glad we could get it working with CSS alone, thanks for all the back and forth!
Looking good:
The icon will grow to larger than 24px if the font-size is larger, but it'll never shrink smaller than that size thanks to the min- rules, so this feels like a safe change to me — it'll only affect sites where someone has deliberately used a larger font, in which case we do want this fix to take effect 👍
LGTM! ✨
Got there in the end. Thanks for your help on this one. 🍺 |
First commit: Apply font size to icon dimensions for the search block In the case of presets, try fetching font size preset using slug from settings
In the PHP file, take into account the custom origin as a priority to match editor behaviour. See: `getUniqueFontSizesBySlug()`
…nts. Made it clear where the origin prioritization comes from in the comments.
escaping incoming attributes
…d font size instead. This is so it works in global styles as well. Temporary bug fix for global styles, which should be done in another PR revert space
Use `em` for search icon.
ramonjd
force-pushed
the
add/search-block-icon-size
branch
from
ccfb9ff
to
3f49b04
Compare
|
@ramonjd Are you looking to have this fix included in WP 6.5? If so please can you add the requisite labels? |
I guess it would be a nice to have. Thanks for the nudge @getdave ! |
ramonjd
added
the
Backport to WP 6.7 Beta/RC
|
I just cherry-picked this PR to the cherry-pick-wp-6-5-beta-3 branch to get it included in the next release: 475fad1 |
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
Use `em` for search icon dimensions. Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: carolinan <[email protected]>
getdave
added
Backported to WP Core
9 tasks
getdave
pushed a commit
that referenced
this pull request
Feb 27, 2024
Use `em` for search icon dimensions. Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: carolinan <[email protected]>
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.
Maybe resolves: #58925
What and how?
Applies font size to icon dimensions for the search block In the case of presets, try fetching font size preset using slug from settings.Adds a width and height of
1.25emto the search icon SVG so that it scales with its parent element's font-size.Why?
When adjusting the font size of a search block with the search icon activated for the button, the search icon remains at
24pxwide and high regardless of the font size.In short, it looks a bit weird when the font size of everything else in the block changes and the icon doesn't.
Testing Instructions
Insert a search block, select the "Use button with icon" option from the toolbar.
Adjust the font size in the block's typography settings.
Here is a sample block!
<!-- wp:search {"label":"Search","showLabel":false,"buttonText":"Search","buttonPosition":"button-inside","buttonUseIcon":true,"style":{"typography":{"fontSize":"45px"}}} /-->Testing Instructions for Keyboard
Screenshots or screencast
2024-02-19.16.33.54.mp4