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

[core] feat(MenuItem): forward MenuProps to submenu #5336

Merged
merged 7 commits into from
Jun 2, 2022

Conversation

bvandercar-vt
Copy link
Contributor

Fixes #5335

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Allow menuProps to be passed to MenuItem that get forwarded to the Menu created within the Popover button.

/**
* Props to spread to the `Menu` within the `Popover`.
*/
menuProps?: Partial<MenuProps> & object;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need & object... that's a weird quirk from an older version of TypeScript that's no longer necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

also I think we should call this prop submenuProps

Suggested change
menuProps?: Partial<MenuProps> & object;
submenuProps?: Partial<MenuProps>;

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 agree with the name change.

So the issue with just Partial<MenuProps> is that MenuProps doesn't have the commonly used prop data-testid. Any Ideas on how we can get that in there?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, there's no good way to do that in the object type. data-* attributes work in JSX because they're built into React. I guess you can leave the & object in this type and leave a note in the JSDoc about why it's there.

Copy link
Contributor Author

@bvandercar-vt bvandercar-vt Jun 1, 2022

Choose a reason for hiding this comment

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

How would you describe that? Like this?
/**
* Props to spread to the child Menu component if this item has a submenu.
* & object allows for data-* attributes to be passed.
*/

packages/core/src/components/menu/menuItem.tsx Outdated Show resolved Hide resolved
packages/core/src/components/menu/menuItem.tsx Outdated Show resolved Hide resolved
bvandercar-vt and others added 2 commits June 1, 2022 09:02
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Co-authored-by: Adi Dahiya <adi.dahiya14@gmail.com>
Comment on lines 82 to 83
*/
menuProps?: Partial<MenuProps> & object;
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it's an implementation detail, so it should be a code comment for devs rather than JSDoc:

Suggested change
*/
menuProps?: Partial<MenuProps> & object;
*/
// note that `& object` is required to allow attributes built-in to JSX like "data-testid"
submenuProps?: Partial<MenuProps> & object;

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 just going to remove the & object, that actually didn't solve it. Just Partial<MenuProps> is fine.

@adidahiya adidahiya changed the title feat: allow custom menuProps in MenuItem for the MenuItem's popover Menu [core] feat(MenuItem): forward MenuProps to submenu Jun 2, 2022
@adidahiya adidahiya merged commit ff056f9 into palantir:develop Jun 2, 2022
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.

need ability to pass custom props to the Menu created in the Menu popover of a MenuItem
2 participants