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

@Menu order should be a double #19320

Closed
Legioth opened this issue May 7, 2024 · 4 comments · Fixed by #19332
Closed

@Menu order should be a double #19320

Legioth opened this issue May 7, 2024 · 4 comments · Fixed by #19332

Comments

@Legioth
Copy link
Member

Legioth commented May 7, 2024

Description of the bug

The properties in the @Menu annotation should correspond to the menu property in Hilla's ViewConfig type where order is a number which corresponds to a double in Java. The purpose of allowing floating point values here is to enable fractional indexing so that you can squeeze in a menu entry between 1 and 2 by setting the order to 1.5 rather than having to re-index all existing menu entires.

Expected behavior

The type of order in @Menu is double

Versions

  • Vaadin / Flow version: 24.4.0.beta1
@tltv tltv self-assigned this May 8, 2024
tltv added a commit that referenced this issue May 8, 2024
Changing `Menu#order` type double and `MenuData#order` type to Double to match it with Hilla's `ViewConfig` type `number`.

Fixes: #19320
@knoobie
Copy link
Contributor

knoobie commented May 9, 2024

@Legioth little issue hijack..

I was wondering if it would be feasible to also add a parent double param to the annotation and the hilla side pushing the menu annotation from a "just for demo purpose" to an "oh that could fit a lot of use cases". The parent double would match with a corresponding index to find the appropriate view parent while building the menu; allowing lose coupling by an ID (order has to be unique anyway) instead of the hard references like layout = Class we've seen previously in Flow.

@Legioth
Copy link
Member Author

Legioth commented May 10, 2024

We have considered hierarchy support for the automatic menu but it hasn't been high enough priority to be implemented yet. I don't think it's a good idea to speculatively add some specific API before we have built the functionality that uses that API.

What we should maybe do instead is to make sure it's easy for applications to add their own custom properties to the menu entry that they can then use in their own menu rendering logic.

@knoobie
Copy link
Contributor

knoobie commented May 10, 2024

Your second approach sounds also reasonable!

mshabarov pushed a commit that referenced this issue May 13, 2024
Changing `Menu#order` type double and `MenuData#order` type to Double to match it with Hilla's `ViewConfig` type `number`.

Fixes: #19320
vaadin-bot pushed a commit that referenced this issue May 13, 2024
Changing `Menu#order` type double and `MenuData#order` type to Double to match it with Hilla's `ViewConfig` type `number`.

Fixes: #19320
mshabarov pushed a commit that referenced this issue May 13, 2024
Changing `Menu#order` type double and `MenuData#order` type to Double to match it with Hilla's `ViewConfig` type `number`.

Fixes: #19320

Co-authored-by: Tomi Virtanen <tltv@vaadin.com>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.beta3 and is also targeting the upcoming stable 24.4.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment