Skip to content

Conversation

@tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Sep 9, 2022

What: Closes #7322
This is a simple implementation of a dropdown using Menu under the hood.
The consumer will pass the toggle in so I did not wrap the menu toggle.

TODO:

@patternfly-build
Copy link
Collaborator

patternfly-build commented Sep 9, 2022

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

Aside from below, one other general thing I've noticed is that the popper menu has a higher z-index than the table of contents (when it is collapsed for narrower windows) which maybe isn't expected? The old dropdown at least was behind it. This would be true for all popper menus, not just here.

onSelect?: (event?: React.MouseEvent<Element, MouseEvent>, itemId?: string | number) => void;
/** Callback to for when the component needs to change the open state of the menu.
* If this is not provided the component will not close the menu when tab or escape are clicked. */
onIsOpenChange?: (isOpen: boolean) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove 'to' from description.
The prop name/description is a little unclear to me. If this is representing the situations when the menu is closing (via keypress or click out) like it appears, would onClose or shouldClose work better?

Or have some extra props and create onTab(isOpen), onEscape(isOpen), and onOutsideClick(isOpen) to break out each separate behavior for the user in case they want finer control? We can also potentially leave the closing behavior as the default and only override it when the props are present rather than the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is a tough one. I named it that because it allows the change the open status as related to the isOpen prop. On close does not describe that. Neither do the onTab(isOpen) or onEscape(isOpen)function names.

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 updated the comment. I don't know if that is more clear.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This looks good @tlabaj . But just a couple nits...

  • I see what the toggle for "Plain menu" is doing, but why would we ever want that? Would it be more useful to show an example of a plain toggle?
  • If we are only going to show one example of a grouped menu, I'd prefer that it use dividers between the groups.

@tlabaj
Copy link
Contributor Author

tlabaj commented Sep 9, 2022

  • I see what the toggle for "Plain menu" is doing, but why would we ever want that? Would it be more useful to show an example of a plain toggle?

I can remove the checkbox. I don't know if it is worth showing the plain toggle since the consumer passes the toggle in to the component.

  • If we are only going to show one example of a grouped menu, I'd prefer that it use dividers between the groups.

I will add dividers.

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Some stuff below is a bit more nitpicky, so let me know what you think. Otherwise I'm really liking this "next" implementation!

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for making those changes @tlabaj !

@tlabaj tlabaj requested a review from kmcfaul September 9, 2022 20:52
@tlabaj
Copy link
Contributor Author

tlabaj commented Sep 12, 2022

I opened a follow up issue for adding testing #7964

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

A couple of things I noticed

  • The menu item text is getting cut off
    • Screen Shot 2022-09-12 at 3 44 21 PM
  • The "with descriptions" dropdown is full width, I'm assuming to accommodate the long menu item text, which overflows the container if it isn't full width:
    • Screen Shot 2022-09-12 at 3 44 29 PM
  • The dropdown classes aren't on all items. IMO I think we should decide whether we want to have individual menu classes (dropdown, select, etc) used to build the menu toggle + menu components or not, and if so, I'd probably advocate to have a dropdown class for every element we have a menu/menu-toggle class for (or maybe just a dropdown class on the parent menu and menu toggle component elements), and for those classes not to reference the existing dropdown styles - that would be new for customizing/theming these "next" components.
    • Screen Shot 2022-09-12 at 3 45 12 PM

I don't think this is a blocker, unless they're easy fixes. I brought up something about consistency between these components below that I think could be used to solve these issues.


Should we apply dropdown styles to the new dropdown? If not, I will remove them.

@tlabaj I can see two sides:

  • Having dropdown classes (either at the parent level, or on everything - the parent, toggle, and menu)
    • makes the "dropdown" unique and can be uniquely themed/styled.
    • gives us the ability to add some code or elements that just work in the dropdown, and don't have to be in the menu or menu toggle any time anyone uses those components, whether they're creating a dropdown or not.
  • Not having classes, and this component basically just being a demo of the toggle + menu that we package and build in default functionality for, keeps it modular, simple, consistent, etc. A user could still add a class to their dropdown if they want to customize something in a dropdown.

I think I would prefer the latter, and just have this be a bundle of the toggle + menu. However it might mean that we likely need to figure out any differences we have between the different types of menus and see if we can normalize them (get rid of the differences), or if we want to keep any of the differences, if we need unique classes to be managed in the CSS for that. For example, one difference that I see that's probably causing an issue in this PR is the difference between the dropdown toggle & menu width versus the select toggle & menu width.

In both menus, the menu will be as wide as the toggle by default, but can grow wider than the toggle if there is a lot of content in the menu. However the dropdown toggle will only be as wide as the content inside of it, and the and the select toggle will fill the width of the container it's in (<MenuToggle isFullWidth />). In this dropdown component, the user has to specify their toggle. I think that implies that - if the default toggle size or whatever is different between the different types of menus, we need to document that default for consistency and rely on users creating it properly (ie, using isFullWidth for the correct menu).

  • Could lead to inconsistencies - unintentional or intentional ones based on personal preference that we wouldn't necessarily recommend
  • If our defaults change, it may be on users to make that change - ie, use another prop or whatever is needed.

We could also do something like use the "dropdown" component to .setProperty() any --pf-c-menu[-toggle]--[MinWidth or whatever] vars to handle any small CSS defaults for these individual components, but then we could be handling a fair amount of CSS in the JS and I'm not sure if that's ideal. Also, I don't know that we could set a --pf-c-menu-toggle--[...] var if the user specifies their own <MenuToggle />?

If these different types of menus presented the same, I think that would mitigate all of this kind of stuff.

--

@nicolethoen nicolethoen merged commit c092c7c into patternfly:main Sep 14, 2022
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.78.0
  • @patternfly/react-catalog-view-extension@4.90.0
  • @patternfly/react-charts@6.92.0
  • @patternfly/react-code-editor@4.80.0
  • @patternfly/react-console@4.90.0
  • @patternfly/react-core@4.239.0
  • @patternfly/react-docs@5.100.0
  • @patternfly/react-icons@4.90.0
  • @patternfly/react-inline-edit-extension@4.84.0
  • demo-app-ts@4.199.0
  • @patternfly/react-integration@4.201.0
  • @patternfly/react-log-viewer@4.84.0
  • @patternfly/react-styles@4.89.0
  • @patternfly/react-table@4.108.0
  • @patternfly/react-tokens@4.91.0
  • @patternfly/react-topology@4.86.0
  • @patternfly/react-virtualized-extension@4.86.0
  • transformer-cjs-imports@4.77.0

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build simple next dropdown

8 participants