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

Documentation on using multi level menus #474

Merged
merged 27 commits into from Apr 27, 2023

Conversation

maximedegreve
Copy link
Contributor

@maximedegreve maximedegreve commented Apr 20, 2023

To support our internal product teams better in incorporating action menus I've updated our docs.
I've also updated our ActionList docs to avoid linking directly to the React components and I've added the nav list there as well.

Note: I still need to update the alt text for the images but I wanted to share this early.

Screenshot of the new docs

Screenshot of the ActionList docs updates

@maximedegreve maximedegreve temporarily deployed to github-pages April 20, 2023 13:13 — with GitHub Actions Inactive
@maximedegreve maximedegreve temporarily deployed to github-pages April 20, 2023 14:51 — with GitHub Actions Inactive
@maximedegreve maximedegreve temporarily deployed to github-pages April 24, 2023 18:23 — with GitHub Actions Inactive
@maximedegreve maximedegreve marked this pull request as ready for review April 24, 2023 20:41
@maximedegreve maximedegreve requested a review from a team as a code owner April 24, 2023 20:41
@maximedegreve maximedegreve temporarily deployed to github-pages April 24, 2023 22:22 — with GitHub Actions Inactive
Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

Some grammar suggestions and a few questions 🙂 excited about multi-level!

content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
width="960"
alt=""
alt="Action menu opened with a label for the trigger (button), overlay (menu), and the internal action list."
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a decision made elsewhere to use ... to indicate opening a dialog? This is a new pattern to me.

screenshot of action menu with a danger button that opens a dialog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This came out of a earlier conversation you shared where it's confusing if a danger action will trigger a confirmation dialog or not.

Both Apple and Microsoft indicate that a dialog will appear by showing a ... at the back of the title. Therefore I think it's good practice to mimic this behaviour.

Screenshot 2023-04-25 at 10 37 54

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem to be a pattern used by macOS and Windows (maybe Linux too?), but I didn't realize "..." indicates a the menu item opens dialog until Max told me. It's not obvious - but I think it's ok to follow this pattern as long as it's not critical for users to know that a dialog will be opened.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it can easily look like truncated text 😅 but I think it's interesting and we should try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm pretty sure that we shouldn't truncate action list items in menus anyway because you can't have tooltips or another way to read what's behind the truncation?

I remember that discussion we had on the command palette.

content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
@maximedegreve maximedegreve temporarily deployed to github-pages April 25, 2023 09:02 — with GitHub Actions Inactive
@maximedegreve maximedegreve temporarily deployed to github-pages April 25, 2023 15:31 — with GitHub Actions Inactive
Action menus are used for disambiguation, navigation, or to display secondary options. They appear when users interact with buttons, actions, or other controls.

[Primer React implementation](https://primer.style/react/ActionMenu)
[Documentation](/components/action-menu)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for the explicit Documentation link, rather than just linking to the component docs from the main heading/title?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure. It's something we don't do on headings right now in the documentation. I guess because they also don't get underlined.

Screenshot 2023-04-27 at 14 35 07

@maximedegreve maximedegreve temporarily deployed to github-pages April 26, 2023 17:11 — with GitHub Actions Inactive
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
content/components/action-menu.mdx Outdated Show resolved Hide resolved
@mperrotti
Copy link
Contributor

Thanks for adding all of this detail! Looks great.

Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

Just a few more wording nitpicks, otherwise looks great!

maximedegreve and others added 5 commits April 27, 2023 14:25
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
Co-authored-by: Mike Perrotti <mperrotti@github.com>
Co-authored-by: Mike Perrotti <mperrotti@github.com>
@maximedegreve maximedegreve merged commit e26616e into main Apr 27, 2023
4 checks passed
@maximedegreve maximedegreve deleted the feature/update-select-menu-docs branch April 27, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants