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

[FluentNavMenu] Show child items via FluentMenu when collapsed #1730

Merged
merged 13 commits into from Apr 8, 2024

Conversation

hksalessio
Copy link
Contributor

@hksalessio hksalessio commented Mar 21, 2024

Pull Request

πŸ“– Description

This pull request adds the ability for the FluentNavMenu to display child items (FluentNavGroup, FluentNavLink) when collapsed. See following image for a better explanation:

image

The feature can be tested in the "Collapsible Navigation Example" section of the "NavMenu" page.

🎫 Issues

In issue #1528, @AlthalusDGr8 asked for this exact feature as it is included in WinUI 3.

πŸ‘©β€πŸ’» Reviewer Notes

This is nothing but a quick, first implementation of this feature.
Although displaying the menu already works, it still needs some refinement and polishing.
For example, the links in the menu are not even clickable yet (i.e. they don't do anything) and I have modified FluentMenu for convenience reasons but I don't know whether this is desired by you.
I guess, most variables should be renamed properly as well.

πŸ“‘ Test Plan

I have not yet checked, whether existing tests fail or succeed.

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

⏭ Next Steps

I will continue to work on this feature over the next few days (maybe weeks, depends on how much time I can find). You are still welcome to review the changes I made up until this point or modify my code in any way.

@hksalessio hksalessio changed the title [FluentNavMenu] Start work on showing child items via FluentMenu when collapsed [FluentNavMenu] Show child items via FluentMenu when collapsed Mar 21, 2024
@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 21, 2024

Hi,
I think this could make for a great addition! To not break current implementations, it should probably be opt-in by setting a specific parameter to true/false. There also needs to be a solution for items that do not have an icon, as they are not shown in the collapsed state.

As you are still working on it, I'll convert the PR to have a 'draft' status.

@vnbaaij vnbaaij marked this pull request as draft March 21, 2024 15:21
@hksalessio
Copy link
Contributor Author

@vnbaaij This is now basically feature complete except for one thing: Should links and groups without an icon on the first layer simply be hidden when the FluentNavMenu is collapsed?

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 28, 2024

Yes, I think those should be hidden when collapsed

@vnbaaij
Copy link
Collaborator

vnbaaij commented Apr 4, 2024

Hi @hksalessio, Any idea when you can wrapthisup? No rush, just curious.

@hksalessio hksalessio marked this pull request as ready for review April 4, 2024 11:59
@hksalessio
Copy link
Contributor Author

Hi @vnbaaij, now :D Looking forward to your review! There are probably still some things to work out. Tests have been verified, though.

Copy link
Collaborator

@vnbaaij vnbaaij left a comment

Choose a reason for hiding this comment

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

Love it. Couple of name changes but nothing shocking.

src/Core/Components/NavMenu/FluentNavMenu.razor.cs Outdated Show resolved Hide resolved
src/Core/Components/NavMenu/FluentNavBase.cs Outdated Show resolved Hide resolved
src/Core/Components/NavMenu/FluentNavGroup.razor Outdated Show resolved Hide resolved
@vnbaaij vnbaaij requested a review from dvoituron April 4, 2024 12:20
@vnbaaij vnbaaij added the feature A new feature label Apr 4, 2024
@vnbaaij vnbaaij added this to the v4.6.1 milestone Apr 4, 2024
@vnbaaij vnbaaij enabled auto-merge (squash) April 8, 2024 09:44
@vnbaaij vnbaaij merged commit 4aedf1e into microsoft:dev Apr 8, 2024
3 checks passed
@vnbaaij
Copy link
Collaborator

vnbaaij commented Apr 8, 2024

@hksalessio Thanks for this. It has been merged!

Can you create an additional PR where you work on accessibility (a11y) challenges for this addition?

@dvoituron
Copy link
Collaborator

@hksalessio About this PR a11y, 2 tasks need to be checked:

  1. The user interface output, using https://accessibilityinsights.io/downloads
  2. Keyboard navigation, to enable selection of a menu item using the Tab or KeyUp/KeyDown keys.

@hksalessio
Copy link
Contributor Author

@vnbaaij @dvoituron I will look into it!

vnbaaij added a commit that referenced this pull request Apr 29, 2024
* [FluentNavMenu] Start work on showing child items via `FluentMenu` when collapsed

* [FluentNavMenu] Add parameter for showing hierarchy when collapsed, add click behavior

* [FluentNavMenu] Hide links and groups without icon when collapsed

* [FluentNavMenu] Update variable names and documentation

---------

Co-authored-by: Alessio Dell Aquila <aquila@hks-systeme.de>
Co-authored-by: Vincent Baaij <vnbaaij@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants