Search block: allow space for input field only when form expanded #54846
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
When a search box has an expandable input field and is in "button only" mode, provide a max-width for the button and take away 100px when the form is expanded to avoid text wrapping and overflow in narrow viewport widths. When the search form is collapsed revert to max-width of 100% to ensure the button fills its container. The input is not there so we don't need the 100px.
ramonjd
added
[Type] Bug
|
Size Change: +20 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1d52132. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6319342183
|
There was a problem hiding this comment.
Thanks for the follow-up @ramonjd! This looks like it's getting closer, and resolves the issue for the text-based button only mode for me. However, it now means that the non-button-only mode no longer receives the max-width calculation, since it doesn't have an aria-expanded attribute.
I'm sure @t-hamano already explored a bunch of potential alternatives before adding the calc() rule, but I wonder if the input field (wp-block-search__input) could use a min-width of 100px — or would that risk overflowing content again? 🤔
andrewserong
approved these changes
Sep 27, 2023
There was a problem hiding this comment.
This is all working nicely for me with the Button only version of the Search block. Apologies for missing that these styles are all scoped, in my earlier review!
This resolves the problem for text based buttons, while preserving the space for the input field in the expanded state 👍
| Before | After |
|---|---|
 |
 |
Thank you for your patience with me! 😄
LGTM! ✨
t-hamano
approved these changes
Sep 27, 2023
There was a problem hiding this comment.
Thanks for the PR!
While working on this problem, there were three problems I wanted to solve in the button-only style:
- Prevent button text wrapping as much as possible to prevent height changes when the form is opened (reported in #53297)
- However, in narrow viewports, the button text needs to wrap properly to prevent overflow.
- Ensure minimum width of input field when in narrow viewport
These issues were resolved in #53373, but I didn't consider scenarios placed inside a flex layout like navigation 😅
This PR appears to ideally solve many scenarios. As @andrewserong said, I think there are very rare edge cases, but I think they can be tolerated.
f89d92c53e1bf12530c2dbb4c9bdaed0.mp4
|
I greatly appreciate the reviews @andrewserong and @t-hamano !! |
|
Thank you very much for the prompt fix @ramonjd and @andrewserong and @t-hamano for testing, even before I had a chance to :) I did notice that when at a very small viewpoint (in this example below, its 320px) within a very narrow container (in the example below, it's a stack variation of the group block), the label will escape That will be a bit of an edge case. |
Thanks @skorasaurus Maybe there's a workaround 😅 It would be great to make work in all viewports. Agree that it's an edge case, but I might spend 20 mins or so figuring out if there's an easy workaround. |
I could reproduce this... for a while, then suddenly I couldn't 😅 Not sure why. My workaround was to remove .wp-block-search.wp-block-search__button-only {
.wp-block-search__button {
margin-left: 0;
max-width: 100%; // not sure if even this is necessary now
}
// Ensure minimum input field width in small viewports.
.wp-block-search__button[aria-expanded="true"] {
max-width: calc(100% - 100px);
margin-left: $button-spacing-x;
}
}But now I can't reproduce on trunk for some reason. Would you have any block code we could test? 2023-09-29.11.23.50.mp4 |
|
No sweat... I thought about it conceptually a little bit and taking a step back, like in that example, I would either (and depending on context, design, etc, no right answer here) : Given that this component is designed to be universal, I'm not sure there's a solution (and I think each developer and designer may have a preference among those options) but I'm mildly curious. |
|
Thanks for the example code! I'm having trouble reproducing the original issue though. 🤔 I agree with your assessment that it'll be hard to come up with a panacea. Hopefully more test cases will come with further usage. Cheers! |
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.
Resolves #54842
What? 🙈
A follow up to #54773
Why? 🙉
How? 🙊
See "What"
Testing Instructions
Some example block code
Screenshots or screencast
With icon as button
2023-09-27.09.03.03.mp4
With button text
2023-09-27.09.02.00.mp4
No text overflow
2023-09-27.09.09.02.mp4