Enable inheritance in search block. #35723
Conversation
jasmussen
added
the
[Block] Search
|
Size Change: +64 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
I agree with this one, though this will affect themes that don't explicitly set a font size for input elements of the search form in particular. On emptytheme, setting the body font size to 30px:
This highlights the need for font control on the theme side for the block. The same block while used on the navigation block will need a very different font size than it will on pages like the search template for example, but I'm not sure we will always want the placeholder and input text to be the same as the body/paragraph font size. At least that's not what I've seen in the designs I've worked with. Generally, the input text is a tad smaller, while the label text is indeed the same as the body/paragraph's. But I'd leave that discussion to the designers, as I said before I agree with this as a starting point (the font-family rule is particularly needed) while being aware that it might affect existing themes. From what I've seen on Blockbase themes (Blockbase doesn't explicitly set a font-size for the block at all), the placeholder text goes from 13.333px (apparently defined by the browser) to whatever the body font size is, which is much bigger. |
|
Excellent catches, thanks for such a great review. I have an idea for handling the case you surface, which I want to try. |
jasmussen
force-pushed
the
try/search-navigation-inheritance
branch
from
1b8fb85 to
6381469
Compare
|
Retrying this one, I actually think what you desribe is what we want: if you set the font size on the body to 30px, so should it be inherited all the way down the line. Navigation already inherits, unless you set otherwise:
I think if it affects themes in a negative way, it should be fixed there. Can you think of any themes that might be affected by this? |
Yeah, I think I agree with you that this is good to have as a base. Ideally, this would be something we could control via theme.json at the very least because I can see how we wouldn't want it to be the case every single time, but it's definitely better than just allowing the browsers to do whatever they want. It would affect all Blockbase themes and it will require some CSS while we have the ability to control this directly on the block, but I'm personally ok with that. Skatepark's search block before and after:
Skatepark's pre footer block pattern also uses the search block:
|
|
Oh, so the search block in blockbase currently doesn't have a font size set?
Setting the font size appears to work for me already:
I'd be happy to keep this PR paused until blockbase has the needed CSS if that's any help. Edit: for what it's worth, the "after" images for Skatepark look better to me 😅 |
No, it doesn't but I'll open a PR right away for it
Oh that's great! maybe I was thinking the typography control would affect the label instead, I don't remember what happened when I last tried this
Nope, don't hold on my account, I'll get it sorted for Blockbase asap. The search block is quite recent and this change will mostly affect block themes mainly so my guess is you could go ahead with this change with few issues |
|
Thanks so much for your thoughtful review! |
Actually, this doesn't work without this PR active, else it will just pick the browser's styles for the input, since the font-size rule is applied to the . |
|
Oh, one more thing before this actually gets merged: right now this works fine with the theme.json rules because it's set to inherit, but it's still more specific than said rules: with this on theme.json: |
|
Excellent sleuthing. I'll dive in again before I merge. |
jasmussen
force-pushed
the
try/search-navigation-inheritance
branch
from
6381469 to
c91308f
Compare
|
Alright, I rebased again, and pushed a fix to lower specificity. You might need to checkout trunk, delete this branch, and check out this branch fresh lest the rebase messes with things. |
|
So the change I made was to simply unscope each of the styles. So instead of
|
Thank you for this. It looks to be working as you mentioned ✅ |
|
The React Native test appears to be consistently failing for reasons unrelated to this PR. I'll leave this one for a bit, then rebase and see if that helps. |
ZebulanStanphill
added
CSS Styling
Description
Font family, size, and line heights, are not inherited in the Search block. That makes its visual appearance stand out when used inside the navigation block. Here's Empty Theme with a search inside nav. Editor:
Frontend:
The font size discrepancy is less visible in Empty Theme, but can be seen here:
This PR adds inheritance, which also makes the editor more like the frontend:
Frontend:
How has this been tested?
Here's some test content:
Please test the search block in a variety of situations, including inside the nav block, and in a few themes, and verify that it always feels harmonious to use.
Checklist:
*.native.jsfiles for terms that need renaming or removal).