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

Add popoverProps property to DropDown component #14867

Merged
merged 1 commit into from
Jun 18, 2019

Conversation

jorgefilipecosta
Copy link
Member

Description

The PopOver component had a property that allows disabling the arrow that appears with it. The DropDown component did not pass that property to the popover, this PR makes sure the DropDown component forwards this property.

How has this been tested?

I changed the DropDown component in packages/block-editor/src/components/block-switcher/index.js and verified the transforms menu did not contain the arrow.

@aduth
Copy link
Member

aduth commented Apr 8, 2019

Is DropdownMenu something we want to be allowed to be shown without the arrow? It seems there's a stronger attachment between the popover and the invoking context than what exists with the Popover component alone.

Is there some discussion or design which motivated this change?

@aduth aduth added [Package] Components /packages/components [Feature] UI Components Impacts or related to the UI component system labels Apr 8, 2019
@aduth
Copy link
Member

aduth commented Apr 8, 2019

In #14851 (fc1fec7), I consider for the URLPopover abstraction to just pass through all additional props to the underlying Popover rather than trying to cherry-pick individual props, since it seemed a potential maintenance burden to try to keep in sync. For URLPopover and Popover however, there's a stronger connection of the abstraction than what might be argued through Dropdown and Popover.

@jorgefilipecosta
Copy link
Member Author

Hi @aduth the use case is the implementation of "menu item" menu proposed in #13690.

image

I can use a PopOver component directly but it seems the correct use case there is a DropDown to avoid having to duplicate functionality implemented in DropDown.

@aduth
Copy link
Member

aduth commented Apr 8, 2019

Thanks for the context. I agree that I could see this as a special variant of a dropdown, though it seems specifically tied to a combination of parts with the outline and arrow button surrounding the "Privacy" text, which makes me think:

  • Should however we choose to abstract this account for all those parts, rather than just exposing the individual noArrow and run the risk that we have many variations of these sorts of designs?
  • Or should this be something specific to the menu dropdown usage? Would it be difficult to style the dropdown in a way which avoids the arrow but doesn't expose it as such from the interface of the component?

I guess the fact that Popover already has this prop could be a reasonable argument in favor of just making it available. The alternative idea to pass through all non-explicitly-handled props to the Popover could work as well and has the advantage of not tacitly condoning it specifically, though in all I'm not really sure passing through props from Dropdown to Popover is something which would be expected from a consumer (at least by default, vs. something separate like popoverProps={ /* ... */ }).

Since there's already a precedent for listing out individual Popover props, I don't see it as necessarily troubling to change from how you've proposed. There's still an option in the future to change this behavior to the full "pass through" behavior.

@afercia afercia requested review from afercia and removed request for afercia April 10, 2019 09:28
@jorgefilipecosta jorgefilipecosta force-pushed the add/noArrow-to-DropDown-component branch 2 times, most recently from a3a6090 to 45d1abd Compare April 10, 2019 19:07
@jorgefilipecosta jorgefilipecosta changed the title Add noArrow property to DropDown component Add popoverProps property to DropDown component Apr 10, 2019
@jorgefilipecosta
Copy link
Member Author

Hi @aduth I like the idea of having a popoverProps property that exposes popover props. I updated the code to follow this logic.

@draganescu draganescu merged commit 3c153f1 into master Jun 18, 2019
@draganescu draganescu deleted the add/noArrow-to-DropDown-component branch June 18, 2019 13:32
@gziolo
Copy link
Member

gziolo commented Jun 18, 2019

There's still an option in the future to change this behavior to the full "pass through" behavior.

Given that some of the props are already propagated explicitly, we probably could re-consider it

I consider for the URLPopover abstraction to just pass through all additional props to the underlying Popover rather than trying to cherry-pick individual props, since it seemed a potential maintenance burden to try to keep in sync.

Yes, it will also make documentation simpler as we would refer to the Popover documentation to learn about available props. At the moment we duplicate documentation for those props which are explicitly passed down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants