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 matching navigation hierarchies with side nav item #7693

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

sissbruecker
Copy link
Contributor

@sissbruecker sissbruecker commented Aug 23, 2024

Adds a matchNested property to vaadin-side-nav-item that allows an item to match nested paths. When enabled, then an item with the path /users does not only match /users, but also /users/1, /users/1/permissions, etc.

For context, the Flow RouterLink and React Router NavLink have similar options:

  • RouterLink allows setting a HighlightCondition that determines whether the link is considered active or not. The most useful default implementations are HighlightConditions.sameLocation (exact match) and HighlightConditions.locationPrefix (match prefix).
  • NavLink has an end property to force exact matching, otherwise it matches by prefix.

Note that both RouterLink and NavLink use prefix matching by default, while side nav item would keep using exact matching by default. For now we keep the default behavior to avoid breaking changes. We can consider aligning the defaults in a future major.

Closes vaadin/flow-components#5227

@knoobie
Copy link
Contributor

knoobie commented Aug 23, 2024

Thanks Sascha! 🙇‍♂️ Votes for 24.5 inclusion

@sissbruecker
Copy link
Contributor Author

We are (theoretically) very close to beta, so I'm not sure. Will bring it up with the team on Monday.

@sissbruecker sissbruecker marked this pull request as ready for review August 26, 2024 08:03
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

We generally avoid boolean properties which default to true as it's problematic:

For a Boolean property to be configurable from markup, it must default to false. If it defaults to true, you cannot set it to false from markup, since the presence of the attribute, with or without a value, equates to true. This is the standard behavior for attributes in the web platform.

If we want to make matchExact the default behavior, then IMO we could instead have a property called noExactMatch or something else so that its default would be false.

packages/side-nav/test/side-nav-item.test.js Show resolved Hide resolved
packages/side-nav/test/side-nav-item.test.js Outdated Show resolved Hide resolved
packages/side-nav/src/vaadin-side-nav-item.js Outdated Show resolved Hide resolved
packages/side-nav/src/vaadin-side-nav-item.js Outdated Show resolved Hide resolved
@sissbruecker
Copy link
Contributor Author

If we want to make matchExact the default behavior, then IMO we could instead have a property called noExactMatch or something else so that its default would be false.

Changed the property to matchNested so that it indicates that it specifically matches nested paths / routes. With that the property is now opt-in, rather than opt-out.

Copy link

sonarcloud bot commented Aug 27, 2024

@vaadin-bot
Copy link
Collaborator

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

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.

[SideNav] Allow matching routes with dynamic parameters
4 participants