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

Enhance Menu to allow grouped items #6162

Merged
merged 9 commits into from Jun 3, 2022

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Jun 1, 2022

What does this PR do?

Deploy Preview

This PR enhances items prop on Menu to allow for grouped items by passing an array of menu item arrays.

Looking for feedback on:

  • API surface
  • Theme structure
  • How separator is implemented as 'top' when index of group is not the first one
API

<Menu
   items={[
      [
         { label: 'View Details' },
         { label: 'Edit Permissions' },
      ],
      [
         { label: 'Delete' },
      ],
    ]}
/>

Theme

  • Adds theme.menu.group.container --> any Box props for an individual group
  • Adds theme.menu.group.separator --> Color and size of separator

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Related to grommet/hpe-design-system#2621

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes.

Should this PR be mentioned in the release notes?

Yes.

Is this change backwards compatible or is it a breaking change?

Backwards compatible.

@taysea taysea marked this pull request as draft June 1, 2022 22:01
@ericsoderberghp
Copy link
Member

Thanks for publishing early for feedback! Can you add an index.d.ts? I find that's a bit easier to review for API surface changes than prop types, especially with object details rather than just object.

I'm good with the array of array of objects, where the objects are 1-1 Button props.

I've been debating whether the heading approach should be built into Menu vs. using DropButton with a custom Drop. There's a line somewhere where adding something that's used rarely but can be done with a more generic approach, like DropButton, makes sense.

If we do go the heading approach, I'm wondering if we should be more structural and do something like:
items={[{ heading: string, items: [{}, ...]}, ...]}

@taysea
Copy link
Collaborator Author

taysea commented Jun 1, 2022

I've been debating whether the heading approach should be built into Menu vs. using DropButton with a custom Drop. There's a line somewhere where adding something that's used rarely but can be done with a more generic approach, like DropButton, makes sense.

I feel like building the headings into Menu could be a valuable addition, especially from the standpoint that you get to leverage all the other menu theming benefits, keyboard/accessibility behavior that was needed for groups, etc. with this. While I don't think headings are the majority case, I do think they're a valid case.

If we decide not to add headings in, we might want to include a detailed code example (either on grommet or ds) of how to achieve both the visuals and the accessibility behavior.

If we do go the heading approach, I'm wondering if we should be more structural and do something like: items={[{ heading: string, items: [{}, ...]}, ...]}

This was also a structure Matt and I ideated on, could be work exploring again to leave items being 1-1 with Button props if that's desirable.

@taysea taysea marked this pull request as ready for review June 3, 2022 04:19
src/js/themes/base.js Show resolved Hide resolved
src/js/components/Menu/Menu.js Outdated Show resolved Hide resolved
src/js/components/Menu/Menu.js Outdated Show resolved Hide resolved
src/js/components/Menu/Menu.js Outdated Show resolved Hide resolved
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

3 participants