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

Components: Refactor DropdownMenu to stop using unstable props #15968

Merged
merged 5 commits into from
Aug 12, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jun 3, 2019

Description

Fixes #15960.

There were 4 unstable props introduced temporarily in #14843:

__unstableLabelPosition,
__unstableMenuClassName,
__unstablePopoverClassName,
__unstableToggleClassName,

They were added for backward compatibility. This PR further explores how to approach it differently.

Proposed changes:

  • __unstableLabelPosition gets replaced with position property in popoverProps prop.
  • __unstablePopoverClassName got implemented as className property in popoverProps props as it seems to be essential for styling of the popover.
  • __unstableToggleClassName and __unstableMenuClassName got remove as they can be expressed as a combination of className property of popoverProps prop and default classes the same UI elements provide. This might be considered as a breaking change but I don't expect it would influence plugins and sites given how specific edge case it is.

This PR introduces new props for DropdownMenu as suggested by @jorgefilipecosta and @youknowriad:

  • Added a new popoverProps prop to the DropdownMenu component which allows passing props directly to the nested Popover component.
  • Added a new toggleProps prop to the DropdownMenu component which allows passing props directly to the nested IconButton component.
  • Added a new menuProps prop to the DropdownMenu component which allows passing props directly to the nested NavigableMenu component.

There are also 2 deprecations introduced:

menuLabel prop in DropdownComponent has been deprecated. Consider using menuProps object and its aria-label property instead.

  • position prop in DropdownComponent has been deprecated. Consider using popoverProps object and its position property instead.

How has this been tested?

There should be no visual changes in the block's More Options menu and in the More Menu.

Screen Shot 2019-06-03 at 17 12 14
Screen Shot 2019-07-26 at 16 28 55
Screen Shot 2019-07-26 at 16 28 58

Screen Shot 2019-06-03 at 17 10 48
Screen Shot 2019-06-03 at 17 10 56
Screen Shot 2019-07-26 at 16 29 11

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Jun 3, 2019
@gziolo gziolo requested a review from aduth June 3, 2019 15:10
@gziolo gziolo self-assigned this Jun 3, 2019
@gziolo gziolo added this to the 5.9 (Gutenberg) milestone Jun 6, 2019
@gziolo gziolo force-pushed the update/refactor-dropdown-menu branch 2 times, most recently from 1bbabd5 to d63b97a Compare June 6, 2019 14:30
@gziolo gziolo requested review from nerrad and ntwb as code owners June 6, 2019 14:30
@nerrad nerrad removed this from the Gutenberg 5.9 milestone Jun 10, 2019
@gziolo gziolo requested a review from a team June 13, 2019 14:25
@@ -67,7 +64,7 @@ function DropdownMenu( {
aria-haspopup="true"
aria-expanded={ isOpen }
label={ label }
labelPosition={ __unstableLabelPosition }
labelPosition={ position }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this change? Can you clarify? It seems that the position prop is meant to be used for something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'd make it a stable api, I'd probably name it tooltipPosition

Copy link
Member Author

@gziolo gziolo Jun 14, 2019

Choose a reason for hiding this comment

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

Yep, but in general, it aligns where you see the popover with where you would expect to see the tooltip :)

I can add tooltipPosition as well but maybe we should introduce toggleProps and contentPropos instead so you could have more control over them. The only issue I see here is that it's not easy to pass an object as a prop because it will cause re-renders every time DropdownMenu re-renders.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the props align, I can see valid use-cases for showing a dropdown under the toggle but the tooltip on top of it. I don't have strong opinions on whether it should be a toggleProps prop or just a position one.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorgefilipecosta added popoverProps prop described as an object for Dropdown components in #14867. There is a bit different handling for Popover component where all remaining props are passed down using rest operator:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/popover/index.js#L267

We probably should optimize APIs to go in one direction moving forward. In this particular case, it feels like using toggleProps would solve the issue we have and keep it more flexible for future use cases.

Copy link
Member

Choose a reason for hiding this comment

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

We probably should optimize APIs to go in one direction moving forward. In this particular case, it feels like using toggleProps would solve the issue we have and keep it more flexible for future use cases.

I think using a toggleProps object as referred seems like a good option.

Copy link
Member Author

Choose a reason for hiding this comment

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

I introduced 3 new props based on the prior work with Dropdown component as implemented in #14867:

  • popoverProps
  • toggleProps
  • menuProps

This allows us to deprecate two props which can be set using this pass-through objects and further simplify public API. For backward compatibility, they are still supported but they will warn on the JS console. I assumed they aren't used widely outside of Gutenberg.

@gziolo gziolo force-pushed the update/refactor-dropdown-menu branch 2 times, most recently from 4cf8206 to ca98123 Compare July 26, 2019 14:21
@gziolo
Copy link
Member Author

gziolo commented Jul 26, 2019

@youknowriad and @jorgefilipecosta - your feedback was addressed and this is ready for final review.

@gziolo gziolo force-pushed the update/refactor-dropdown-menu branch from ca98123 to 46bef32 Compare August 8, 2019 09:52
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Works well on my tests 👍

packages/components/src/dropdown-menu/index.js Outdated Show resolved Hide resolved
@gziolo gziolo force-pushed the update/refactor-dropdown-menu branch from 46bef32 to bb25919 Compare August 12, 2019 12:28
@gziolo gziolo added this to the Gutenberg 6.3 milestone Aug 12, 2019
@gziolo gziolo merged commit e2c01f7 into master Aug 12, 2019
@gziolo gziolo deleted the update/refactor-dropdown-menu branch August 12, 2019 12:44
gziolo added a commit that referenced this pull request Aug 29, 2019
* Remove __unstableLabelPosition by reusing position provided for the dropdown menu

* Replaces unstable class names with the documented contentClassName prop

* Refactor DropdownMenu component to allow passing props to nested components

* Add CHANGELOG entries and deprecations messages for update props

* Swap params for mergeProps helper
gziolo added a commit that referenced this pull request Aug 29, 2019
* Remove __unstableLabelPosition by reusing position provided for the dropdown menu

* Replaces unstable class names with the documented contentClassName prop

* Refactor DropdownMenu component to allow passing props to nested components

* Add CHANGELOG entries and deprecations messages for update props

* Swap params for mergeProps helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Components: Stop using unstable props in DropdownMenu
4 participants