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

Update submenu font-size inheritance to fix scaling of top level submenu parent #39277

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

Humanify-nl
Copy link

@Humanify-nl Humanify-nl commented Mar 8, 2022

Description

Resizing the font variable on a submenu will affect the top-level menu item (parent of submenu). This fix changes the .wp-block-navigation-submenu to inherit its font-size properly, and only scale the submenu itself to the variable set in theme.json.

Reference: 39263

Testing Instructions

  1. Go to theme.json
  2. Under styles { block }, and set a font-size for core/navigation-submenu that differs from the top level menu
  3. See the font-size change in the submenu, and its parent anchor.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

Resizing the font variable on a submenu will affect the top-level menu item (parent of submenu). This fix changes the .wp-block-navigation-submenu to inherit its font-size properly, and only scale the submenu itself to the variable set in theme.json.
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 8, 2022
@github-actions
Copy link

github-actions bot commented Mar 8, 2022

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Humanify-nl! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@jasmussen
Copy link
Contributor

Thank you for the PR, I took it for a spin. Here's the nav block with no theme.json changes applied:
after, no theme json change

Here's with font size changed for both navigation items and submenu items:

after, theme json change

This is from my theme.json:

	"styles": {
		"blocks": {
			"core/navigation-link": {
				"typography": {
				  "fontSize": "8px"
				}
			},
			"core/navigation-submenu": {
				"typography": {
				  "fontSize": "8px"
				}
			},
		}
	}

Here's the trunk branch with those same theme.json changes applied:

before, theme json changes

Note that in both of the above examples, I see the submenu item font size correctly inherited from the parent font size. So your added rule in this PR seems harmless, but I'm also wondering if it's unnecessary, since I can't reproduce the original issue?

There's a separate issue here: when using theme.json to style the navigation and submenu items directly like the above theme.json suggests, then the navigation block font size controls becomes inoperative. I think this is intentional insofar as theme.json should allow that. But if instead you applied the following to theme.json, it'd still work:

	"styles": {
		"blocks": {
			"core/navigation": {
				"typography": {
				  "fontSize": "8px"
				}
			},
		}
	}

trunk, different style

I sense that your use case might be to have one font size applied to top level items, and another for children, is that right? In that light, the following theme.json might be intuited to work as such:

			"core/navigation-link": {
				"typography": {
				  "fontSize": "20px"
				}
			},
			"core/navigation-submenu": {
				"typography": {
				  "fontSize": "8px"
				}
			},

But that results in this:

Screenshot 2022-03-09 at 08 53 32

Not very usable at all, but technically correct, since navigation-link blocks are used as children of the navigation-submenu block.

I do think it's a completely valid question whether we shouldn't support additional style controls for submenu items. #39205 is not directly related, but touches on it. But ultimately this whole question makes me think we should get a few additional thoughts on how to best accommodate this use case. @tellthemachines if you have time to look, I'd appreciate your insights!

Thanks again for the PR.

@Humanify-nl
Copy link
Author

Humanify-nl commented Mar 9, 2022

Thank you for the feedback.

My theme.json was a slightly different use case, where I did not set the font-size on the navigation-link, but on the navigation block itself, plus on the navigation-submenu

My theme.json:

core/navigation { typography { font-size: 2rem }}
core/navigation-submenu {typography { font-size: 1,5rem }}

I look at this from a CCS logic. The navigation is the parent container which has one size (for all children inside) assigned as default in theme.json.

When I set a font size for the submenu additionally, via theme,json, I would expect that only submenu would adopt this size, and not the top level link itself.

But what I see in your tests above is different,

So if I understand correctly; if I would want the result I’m aiming for, I have to set the navigation-submenu and the navigation-link. Instead of navigation + navigation-submenu.

Assumed hierarchy

So my assumed hierarchy of overriding within theme.json:

navigation < navigation-submenu < navigation-link ; but I see from your test this is not valid.

It is instead (if I understand correct):

Either set navigation-submenu & navigation-link OR only on the navigation itself.

(and overriding any of this in block controls inside the editor should override any of the value set in theme.json)

How should it work?

This is how I would expect it to work (pure theme.json)

  • when setting navigation only, it will set all sizes
  • when setting navigation-submenu only, it will only set submenu sizes
  • when setting navigation-link only it will do the same as setting in navigation (links are in sub or main menu)
  • when setting -submenu and -link, -submenu links should take higher specificity, -link would only target top-level
  • when setting navigation & -submenu , and not -link. Submenu overrides navigation top level size
  • when setting navigation, -submenu & -link, navigation settings are fully overridden.

Hope this helps!

As an additional edit: let me illustrate this HTML to you, because initially I thought this was the culprit:

image

The font-size that is specified by the value of navigation-submenu (set in theme.json), is placed on the class: .navigation-submenu. While that class is on the li element. So when a submenu size is set, it should not target this class (because it adds a value to top-level item instead of submenu), but the child ul instead: .wp-block-navigation__submenu-container.

image

Here we see that value here on the wrong css-class (I added my own custom css var in theme.json, but this shouldnt matter, and disabled my own inherit edit to demonstrate).

@jasmussen
Copy link
Contributor

When I set a font size for the submenu additionally, via theme,json, I would expect that only submenu would adopt this size, and not the top level link itself.

So if I understand correctly; if I would want the result I’m aiming for, I have to set the navigation-submenu and the navigation-link. Instead of navigation + navigation-submenu.

Yes, this seems like the key issue to solve. It feels perfectly valid to want to affect all items inside a submenu, only. With your approach, I get this:

Screenshot 2022-03-09 at 12 03 14

That's close, in that navigation-link items inside the submenu do inherit the font size of the parent submenu container.

But if we are to let the top level item have a larger font-size, like its siblings, then navigation-links inside would inherit that larger font size, putting us back to square one.

I wonder if the best solution here isn't to allow font-size controls on both menu items and submenu items, a la in #39205 — then you could specifically set those, at least.

To my understanding, theme.json isn't meant to have any form of cascading system, so you could target for instance just children of a particular block type. Correct me if I'm wrong, @jorgefilipecosta?

@Humanify-nl
Copy link
Author

Humanify-nl commented Mar 9, 2022

Even if theme.json shouldn’t cascade, it’s values should at least be put on the element that it supposedly targets. Right?

Forgive me for being foolhardy about this, but I still don’t see any good reason why the submenu fontsize variable via theme.json (submenu) is placed on the li (parent)element and not the ul of the submenu. I don’t see any use-case for it. Why would anyone want to change that specific top level item if all we want is resize the submenu text and leave the rest as is.

@scruffian
Copy link
Contributor

To my understanding, theme.json isn't meant to have any form of cascading system, so you could target for instance just children of a particular block type.

I believe this is incorrect. Theme.json gives you control over the CSS for a block, so everything inside that block (including inner blocks) would inherit those rules.

@jasmussen
Copy link
Contributor

Thanks for the sanity check. That does seem to suggest it was correct to close #37528 then, right?

@scruffian
Copy link
Contributor

Thanks for the sanity check. That does seem to suggest it was correct to close #37528 then, right?

Complicated question!

@scruffian
Copy link
Contributor

I still don’t see any good reason why the submenu fontsize variable via theme.json (submenu) is placed on the li (parent)element and not the ul of the submenu.

The reason for this is that the theme.json targets the block class, which in this case is wp-block-navigation-submenu. As you say this class is being applied to the li above the submenu, which seems like the wrong approach to me, but I'm new to this block so I might be missing some context. I'll keep looking into it...

@Humanify-nl
Copy link
Author

Humanify-nl commented Mar 11, 2022

The reason for this is that the theme.json targets the block class, which in this case is wp-block-navigation-submenu. As you say this class is being applied to the li above the submenu, which seems like the wrong approach to me, but I'm new to this block so I might be missing some context. I'll keep looking into it...

Context might be: something to do with the fact that when in the editor and transforming a link to a submenu, it replaces the link block with a submenu block.

@scruffian
Copy link
Contributor

I created a proposal for a potential way forward here

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

To my understanding, theme.json isn't meant to have any form of cascading system, so you could target for instance just children of a particular block type.

I believe this is incorrect. Theme.json gives you control over the CSS for a block, so everything inside that block (including inner blocks) would inherit those rules.

Well for now we don't plan on having a system where one can target block B when it is nested inside block A. That makes all the systems more complex, but in the future, we may have it, if there are compelling reasons.

@Humanify-nl
Copy link
Author

I think it’s important that this behavior is made very clear in the theme.json handbook. Because its probably not uncommon to expect this from a front-end mindset, especially when it’s values are very css.

If I can assist in any way, let me know how.

@mtias mtias added the [Block] Navigation Affects the Navigation Block label Mar 28, 2022
@kathrynwp
Copy link

@tellthemachines @jorgefilipecosta Hey there - did you have any thoughts on how to move this PR forward? Thanks for having a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants