feat: theme property for individual menu items #2401
Conversation
The vaadin-menu-bar as a whole is themable, and setting the theme attribute on it will propagate it to all menu bar buttons. However, a user might want to add a theme to only some select buttons, for example to show one as the primary option. While this can be done by supplying a component, it is not the most convenient approach, and the theme property is also needed for the Java API. Part of vaadin/flow-components#880.
|
FYI @yuriy-fix |
As the context menu used by vaadin-menu-bar propagates the parent element theme to context menu items, a similar approach must be used here. Otherwise, whatever theme is manually set will be overridden.
|
Thought I could get away without introducing the same for |
There was a problem hiding this comment.
As mentioned in the PR description, I guess we should override theme coming from the parent.
It will work for cases when you want to have one tertiary button and others as primary
In addition, we can also provide an array / list of themes (themes:['error', 'primary']) for an item in that case.
The reason is that it won’t be easy to remove the theme from the child items and users will need to come up with CSS tricks in order to re-style the item with a weird combination of themes, like mentioned primary + tertiary
eriklumme
force-pushed
the
feat/menu-bar-item-theme
branch
from
593c5f9
to
2a5dd61
Compare
|
@web-padawan might want to take a look at the merge commit, that I'm not ruining his changes... (If you click |
| this._buttons.forEach((button) => { | ||
| const itemTheme = this.__stringOrArrayToString(button.item && button.item.theme); | ||
| if (itemTheme) { | ||
| button.setAttribute('theme', itemTheme); | ||
| } else if (theme) { | ||
| button.setAttribute('theme', theme); | ||
| } else { | ||
| button.removeAttribute('theme'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
This logic should be removed from here as we now handle it in the other mixin - no need to duplicate.
So just removing the lines below should be enough (and only set sub menu theme).
There was a problem hiding this comment.
Fixed, and the menu-bar tests in web-components and flow-components still pass, so looks good.
packages/vaadin-context-menu/src/vaadin-contextmenu-items-mixin.js
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
|
Menu-bar tests flakiness is unrelated to this PR, I have restarted the build and when it's green I will approve. |
Backported changes for Vaadin 14 from vaadin/web-components#2401. Part of vaadin/flow-components#880.
Backported changes for Vaadin 14 from vaadin/web-components#2401. Part of vaadin/flow-components#880.
Backported changes for Vaadin 14 from vaadin/web-components#2401. Part of vaadin/flow-components#880.
|
This ticket/PR has been released with platform 22.0.0. |
The vaadin-menu-bar as a whole is themable, and setting the theme attribute on it will propagate it to all menu bar buttons.
However, a user might want to add a theme to only some select buttons, for example to show one as the primary option. While
this can be done by supplying a component, it is not the most convenient approach, and the theme property is also needed
for the Java API.
Part of vaadin/flow-components#880.
Note: I am not sure if it's better to merge the themes of the menu-bar and the menu-bar-item, or if the menu-bar-item should override the former. Currently they are merged.
Merging makes sense to me, for example if you set
primaryon the menu-bar andspecialon one item, it will beprimary special.However, what if you wanted all but one to be
primary? You can't remove the menu-bar theme by settingtheme: ''on an item. And if you do something liketheme: 'tertiary', the result will beprimary tertiary, and the styles will be strange. So this requires you to customize the styles for that combination.Thoughts?
Type of change
Checklist
Additional for
Featuretype of change