Add submenuItems support to HeaderBar#5068
Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @aleksip! See below for some first-round suggestions.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the improvements, @aleksip -- just one last question about config naming.
| # - routeParams: parameters for dynamic routes | ||
| # - url: target url, alternative to route - required unless route or template | ||
| # is set | ||
| # - name: name of an item, used as a CSS class for the menu item's <li> tag |
There was a problem hiding this comment.
Any reason not to call this class or className rather than name if that's its only purpose?
There was a problem hiding this comment.
Just to align with AccountMenu and AdminMenu, which use it for a similar purpose. Happy to rename if you would prefer class or className.
There was a problem hiding this comment.
I agree that consistency is desirable. Unfortunately, it looks like we already have some inconsistency in AccountMenu, where we have a top-level class property and then use name in the lower levels. I guess we're stuck with "name" though, since it has been like this for years and we can't change it without breaking back-compatibility and creating more work to update config files. I'm sorry that I didn't notice this earlier! I just don't like "name" because it has a specific technical meaning in HTML, and that makes the current use as a class feel confusing to me.
Let me know if you have any further thoughts/preferences. I'll merge this as-is unless you have any further thoughts on how to improve it.
At very least, though, we should update the other .yaml configs so that all the comments on the name setting are consistent. I'll leave it up to you whether to do that here or as a follow-up.
There was a problem hiding this comment.
I have updated the other configs. It looks like the key is used for several things in AccountMenu so maybe it could be thought of as a multi-purpose key, as long as the documentation is kept up to date.
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @aleksip, all looks reasonable to me now!
2 participants
Follow-up to #5040