Skip to content

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.

@jorgefilipecosta jorgefilipecosta force-pushed the add/noArrow-to-DropDown-component branch from 45d1abd to b104f48 Compare June 14, 2019 19:11
@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.

6 participants