Skip to content
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

Archeo: add CSS for mobile menu font size #5644

Merged
merged 3 commits into from
Mar 11, 2022
Merged

Conversation

mikachan
Copy link
Member

@mikachan mikachan commented Mar 9, 2022

Changes proposed in this Pull Request:

This increases the font size of the main navigation on small screens, as suggested in #5631:

Before After
image image

The blockGap has already been reduced to var(--wp--custom--spacing--small) in #5640. This is what I've used in the above screenshot. Should this be reduced any further?

Related issue(s):

#5631

@mikachan mikachan added the [Theme] Archeo Automatically generated label for Archeo. label Mar 9, 2022
@mikachan mikachan added this to the Archeo milestone Mar 9, 2022
@mikachan mikachan requested review from kjellr and a team March 9, 2022 17:28
@mikachan mikachan self-assigned this Mar 9, 2022
@mikachan mikachan mentioned this pull request Mar 9, 2022
4 tasks
archeo/style.css Outdated
* Possible future fix: https://github.com/WordPress/gutenberg/pull/33543
*/
@media (max-width: 599px) {
.wp-block-navigation__responsive-container-content li {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed this selector doesn't work in the editor for non-standard links. For example, I have Submenu and a Home link, and the large font size isn't applying:
Screen Shot 2022-03-09 at 2 12 32 PM

It makes me wonder if adding this CSS is worth it, or if it's going to cause more confusion if a user changes this navigation's font size in the editor and previews it.

Another thought — could we create a custom CSS variable that uses a responsive type size and apply it to the navigation block here. That way any user modifications to the font-size would still take precedence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we create a custom CSS variable that uses a responsive type size and apply it to the navigation block here.

Oh that's a fine idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the CSS variable with a responsive type size is a great idea, however, I'm not sure how to do this where the font size is bigger on smaller screens and smaller on larger screens.

For now, I've gone with setting the larger font size for the overlay in CSS (we were already doing this, I missed it when I was working on this PR!) and setting the default font size in theme.json.

@kjellr
Copy link
Contributor

kjellr commented Mar 9, 2022

If this doesn't already exist, we should file an issue to ask for the ability to specify an overlay-specific font size. I imagine this will be a common use case, and we already have the ability to set overlay-specific values for color via overlayBackgroundColor and "overlayTextColor.

@pbking
Copy link
Contributor

pbking commented Mar 10, 2022

I don't see the responsive menu "small" as posted in the before shots, I looks correct as of this change. The only change I can see from this PR is that the font size configuration has been moved from the BLOCK to THEME.JSON which looks good to me.

@mikachan
Copy link
Member Author

The only change I can see from this PR is that the font size configuration has been moved from the BLOCK to THEME.JSON

Yeah this sounds correct, I missed that we'd already set the large font size for the responsive menu via CSS, but then it looks like it was being overwritten by the font size set in the header markup. So, I've removed the font size from the header.html and moved it to theme.json.

Going to look for / create an issue for setting an overlayFontSize (or similar) setting in GB now..

@pbking
Copy link
Contributor

pbking commented Mar 10, 2022

I couldn't find a related issue so have opened this: WordPress/gutenberg#39352

The change looks good, 🚢

@jffng
Copy link
Contributor

jffng commented Mar 10, 2022

Going to look for / create an issue for setting an overlayFontSize (or similar) setting in GB now..

Looks like some folks are starting to think about this holistically here: WordPress/gutenberg#39142

@jffng jffng merged commit 42321fb into trunk Mar 11, 2022
@jffng jffng deleted the archeo/update-mobile-menu branch March 11, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Theme] Archeo Automatically generated label for Archeo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants