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(NcActions): submenus #6209

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

feat(NcActions): submenus #6209

wants to merge 4 commits into from

Conversation

ShGKme
Copy link
Contributor

@ShGKme ShGKme commented Nov 12, 2024

☑️ Resolves

Add submenu feature:

  • NcActionButton's is-menu is now not only for an icon, but actually to handle a specific menu
  • #submenu:NAME slot is uses to pass a specific submenu

Takes into account:

  • Multi-level menu stack
  • Adding "Back" button to return from a submenu
  • Escape closes submenu instead on an entire menu
  • After opening a menu the first element is on focus
  • After closing submenu the focus returns to the menu button

To discus:

  • submenu vs menu name for slot
  • Add a new prop in pair to is-menu because it's already called like a boolean flag

🖼️ Screenshots

Docs

image

In action

NcActions-Submenu

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

Signed-off-by: Grigorii K. Shartsev <[email protected]>
@ShGKme ShGKme added enhancement New feature or request 3. to review Waiting for reviews feature: actions Related to the actions components labels Nov 12, 2024
@ShGKme ShGKme self-assigned this Nov 12, 2024
@ShGKme ShGKme added this to the 8.21.0 milestone Nov 12, 2024
@skjnldsv
Copy link
Contributor

Design team told me some time ago that "back" should not be used, but rather the name of the parent option :)

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 12, 2024

Design team told me some time ago that "back" should not be used, but rather the name of the parent option :)

Files Talk
image image

Indeed. But looks weird for me. We are in a "Setting reminder menu", and the first menu button is "Set reminder". Click on "Ser reminder" sets nothing and goes back to file actions. This is also a11y issue.

cc @nextcloud-libraries/designers

@skjnldsv
Copy link
Contributor

Indeed. But looks weird for me. We are in a "Setting reminder menu", and the first menu button is "Set reminder". Click on "Ser reminder" sets nothing and goes back to file actions. This is also a11y issue.

This was a direct request from @jancborchardt. Make sure we get his approval before changing that behaviour please :)

Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Tested, waiting for design decision

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 12, 2024
@marcoambrosini
Copy link
Contributor

marcoambrosini commented Nov 13, 2024

Nice job standardising this!

Agree that the word "back" works best in this case. "set reminder" sounds like another action, while all the button is doing is literally bringing us back. I agree with @ShGKme point

Update: Hidden as not strictly related.

That said, I think this UI should only be a mobile fallback and we should have proper cascading menus like everybody else. It's way more intuitive to hover or tab through options than having to dive in and dive back out.

Examples

Microsoft 365
Screenshot 2024-11-13 at 10 46 41

Google drive
Screenshot 2024-11-13 at 10 46 11

Slack
Screenshot 2024-11-13 at 10 55 06

Zoom
Screenshot 2024-11-13 at 10 55 28

@ShGKme ShGKme added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 13, 2024
@ShGKme ShGKme marked this pull request as draft November 19, 2024 21:59
@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 19, 2024

Converting to draft during design discussions in the issue

@marcoambrosini
Copy link
Contributor

marcoambrosini commented Nov 20, 2024

Converting to draft during design discussions in the issue

@ShGKme I opened a separate issue for the cascading proposal, I think what you've done is still useful for mobile. Don't you think?
And even for desktop it improves the current state, so I wouldn't want to block it

@ShGKme
Copy link
Contributor Author

ShGKme commented Nov 20, 2024

@ShGKme I opened a separate issue for the cascading proposal, I think what you've done is still useful for mobile. Don't you think?
And even for desktop it improves the current state, so I wouldn't want to block it

I'm not sure, because if we go with the cascade, implementation should be different from both HTML (a11y) side and vue component interface.

Something like that

<NcActions>
  <NcActionButton>Option 1<NcActionButton>
  <NcActionButton>Option 2<NcActionButton>
  <NcActionSubmenu label="Option 3">
    <NcActionButton>Option 3.1<NcActionButton>
    <NcActionButton>Option 3.2<NcActionButton>
  <NcActionSubmenu>
</NcActions>

This would be cool to support both views (e.g. desktop/mobile switch). but we'd need to think about the implementation, especially with our quite complex NcActions component.

@Antreesy Antreesy modified the milestones: 8.21.0, 8.22.0 Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: actions Related to the actions components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NcActions] Proposal: submenu support
5 participants