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

feat: allow theming individual menu items #2066

Merged
merged 7 commits into from
Sep 23, 2021
Merged

Conversation

eriklumme
Copy link

@eriklumme eriklumme commented Aug 30, 2021

This change enables theming individual menu bar items, for example to display one item as the primary item. As the menu bar uses the context menu to display submenus, most of the changes are in there.

Depends on vaadin/web-components#2401

Part of #880

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@eriklumme
Copy link
Author

Some concerns:

  • I don't use the HasTheme interface, because it extends HasElement. If I extended that, I'd have to most, if not all, of the 9 methods it defines. Alternatively I could create my custom ThemeList, but ThemeListImpl has no default constructor, and I'd also have to override setThemeName and getThemeName then. Does it have to implement HasTheme, and if so, which approach is preferable?
  • I added a simple demo to the menu bar, as the existing MenuBarVariants seem to work on the main menu buttons. They don't work for the context menu items, though, so I added no example for that. I figured if and when variants are added for submenu, then examples could be added to MenuBar and/or ContextMenu.

@eriklumme
Copy link
Author

Note that this PR depends on vaadin/web-components#2401

@vaadin vaadin deleted a comment from vaadin-bot Sep 15, 2021
Erik Lumme and others added 6 commits September 21, 2021 15:25
This change enables theming individual menu bar items, for example to display one item as the primary item. As the menu bar uses the context menu to display submenus, most of the changes are in there.
Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

@yuriy-fix My concern here is that this custom implementation of the theming API differs from the custom one we already have in MessageListItem (see here):

  • here we implement the setThemeName and getThemeName from HasTheme
  • in MessageListItem we implement addThemeNames, removeThemeNames, and hasThemeName from HasTheme

Personally I think we should align this:

  • we already have quite a few DX issues with differing APIs in Flow components
  • here we have a chance to improve on that with just a little bit more effort, and taking a little bit more time
  • once we introduce an API it becomes a pain to change it, customers will be angry

Since MessageListItem already has a stable API, it would be much easier to change the API in this PR, so I would suggest to do that.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 21 issues

  • CRITICAL 19 critical
  • MINOR 2 minor

Top 10 issues

  1. CRITICAL MenuBarView.java#L61: Define a constant instead of duplicating this literal "Selected: " 10 times. rule
  2. CRITICAL MenuBarView.java#L63: Define a constant instead of duplicating this literal "Project" 7 times. rule
  3. CRITICAL MenuBarView.java#L64: Define a constant instead of duplicating this literal "Account" 6 times. rule
  4. CRITICAL MenuBarView.java#L65: Define a constant instead of duplicating this literal "Sign Out" 11 times. rule
  5. CRITICAL MenuBarView.java#L68: Define a constant instead of duplicating this literal "Users" 8 times. rule
  6. CRITICAL MenuBarView.java#L69: Define a constant instead of duplicating this literal "Billing" 6 times. rule
  7. CRITICAL MenuBarView.java#L76: Define a constant instead of duplicating this literal "Invoices" 12 times. rule
  8. CRITICAL MenuBarView.java#L77: Define a constant instead of duplicating this literal "Balance Events" 12 times. rule
  9. CRITICAL MenuBarView.java#L80: Define a constant instead of duplicating this literal "Edit Profile" 16 times. rule
  10. CRITICAL MenuBarView.java#L82: Define a constant instead of duplicating this literal "Privacy Settings" 16 times. rule

Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Looks good.

Just one thing to verify: I assume that the menu bar theme variants can only be applied to root items / buttons, and not to sub items showing up in the context menu - at least the latter did not work from my testing.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.alpha5 and is also targeting the upcoming stable 22.0.0 version.

@tltv tltv mentioned this pull request Nov 9, 2021
9 tasks
alvarezguille pushed a commit that referenced this pull request Nov 10, 2021
This change enables theming individual menu bar items, for example to display one item as the primary item. As the menu bar uses the context menu to display submenus, most of the changes are in there.

* feat: allow theming individual menu items

Backported changes for Vaadin 14 from
vaadin/web-components#2066.

Depends on vaadin/vaadin-menu-bar#140.

Part of
#880.

* Bump vaadin-context-menu 4.6.0-alpha1 and vaadin-menu-bar  1.3.0-alpha1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants