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

feat(OverflowMenu): Use next dropdown and dropdownItems #8359

Merged
merged 21 commits into from Mar 17, 2023

Conversation

tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Nov 10, 2022

What: Closes #8071

Updated the OverflowMenu to

  • Use next DropdownItems under the hood.
  • Use next Dropdown in the examples.

Additional issues:

@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 10, 2022

@nicolethoen
Copy link
Contributor

Looks like the error is a typescript issue:
Screenshot 2023-01-09 at 1 30 34 PM

@@ -16,6 +16,7 @@ propComponents:
import AlignLeftIcon from '@patternfly/react-icons/dist/esm/icons/align-left-icon';
import AlignCenterIcon from '@patternfly/react-icons/dist/esm/icons/align-center-icon';
import AlignRightIcon from '@patternfly/react-icons/dist/esm/icons/align-right-icon';
import EllipsisVIcon from '@patternfly/react-icons/dist/esm/icons/ellipsis-v-icon';

Copy link
Contributor

Choose a reason for hiding this comment

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

Add imports here before they're used in examples below

Suggested change
import { Dropdown, DropdownList } from '@patternfly/react-core/next';

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

By using an alias for Dropdown to call it DropdownNext in the examples, are we setting a precedent that will make updating the imports in the future harder for doing, with codemods for example, for people who copy that pattern and later want to switch from the next dropdown to the official dropdown, etc?

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.

Looks good! I am noticing that in the examples, the menu width is only as wide as the kebab toggle, so all but the first letter the menu items get cut off. We could pass minWidth to the DropdownNext in the examples or expose the popperMatchesTriggerWidth prop in the Next Dropdown component to set it on Popper.

@tlabaj
Copy link
Contributor Author

tlabaj commented Feb 10, 2023

popperMatchesTriggerWidth

@mcoker what are your thoughts on this?

@mcoker
Copy link
Contributor

mcoker commented Feb 13, 2023

@tlabaj since it's a dropdown menu, the menu should ideally have a min-width of the toggle width, but should grow to fit the size of the menu content if the content is wider than the toggle.

@tlabaj
Copy link
Contributor Author

tlabaj commented Feb 15, 2023

@thatblindgeye @mcoker this issue has been opened to address menu widths #8682

Should I leave this as is for now or add a width in the interim?

@mcoker
Copy link
Contributor

mcoker commented Feb 15, 2023

@tlabaj IMO leave it as is, and update components that use popper to have the appropriate size set either as part of #8682, or maybe separate tasks for each thing that needs to be updated. I may be misunderstanding, but that issue says it will set defaults for <Menu>, so that may take care of this since it's using dropdown-next, which uses menu?

@tlabaj
Copy link
Contributor Author

tlabaj commented Feb 15, 2023

I may be misunderstanding, but that issue says it will set defaults for <Menu>, so that may take care of this since it's using dropdown-next, which uses menu?

@mcoker correct

@tlabaj
Copy link
Contributor Author

tlabaj commented Mar 3, 2023

Using the Dropdown as DropdownNext alias like other examples seems like it might be a temporary fix).

@erik @nicolethoen Correct. the alias is a temp solution until we deprecate old dropdown. Since there is an issue in the framework.

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.

The above demos look to be loading now.

The below comment would also apply to Table's "Composable: Actions Overflow" example, as it has a similar class/styling issue.

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.

Reapproving for the above comments from Sarah and myself 🎉

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 to me. Is there something specific I should be looking for?

@tlabaj
Copy link
Contributor Author

tlabaj commented Mar 17, 2023

Looks good to me. Is there something specific I should be looking for?

@mcarrano Just that the overflow menu appears as expected with the Next dropdown 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 to me. Is there something specific I should be looking for?

@mcarrano Just that the overflow menu appears as expected with the Next dropdown implementation.

It does!

@thatblindgeye thatblindgeye merged commit d356f4a into patternfly:v5 Mar 17, 2023
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.38
  • @patternfly/react-core@5.0.0-alpha.38
  • @patternfly/react-docs@6.0.0-alpha.41
  • demo-app-ts@5.0.0-alpha.21
  • @patternfly/react-integration@5.0.0-alpha.8
  • @patternfly/react-table@5.0.0-alpha.38

Thanks for your contribution! 🎉

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

Successfully merging this pull request may close these issues.

Overflow menu - Refactor overflow menu to use the next dropdown
8 participants