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.

Sign up for GitHub

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

Jump to bottom

feat(Menu): match css structure #8820

Merged
merged 2 commits into from Apr 10, 2023
Merged

feat(Menu): match css structure #8820

merged 2 commits into from Apr 10, 2023

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Mar 16, 2023

What: Closes #8187

  • Renames MenuInput to MenuSearch
  • Adds MenuSearchInput

@patternfly-build
Copy link
Contributor

patternfly-build commented Mar 16, 2023 •
edited

Preview: https://patternfly-react-pr-8820.surge.sh

A11y report: https://patternfly-react-pr-8820-a11y.surge.sh

thatblindgeye
thatblindgeye reviewed Mar 17, 2023
View reviewed changes
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.

CSS-wise looks good. Had some React comments below. Additionally just needs a codemod issue opened for any applicable changes being made.

packages/react-core/src/components/Menu/MenuSearchInput.tsx Show resolved Hide resolved
packages/react-core/src/demos/ComposableMenu/examples/ComposableDropdwnVariants.tsx
Comment on lines +130 to +132
<MenuSearch>
<MenuSearchInput>Unselectable text displayed at the top of the menu</MenuSearchInput>
</MenuSearch>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would depend how common this is (having a non-menu item content above the menu that isn't a search input), but in this instance I feel like MenuSearch/MenuSearchInput aren't really accurate for the context.

For something like this I wonder if a MenuHeader would be better (since we do already have a MenuFooter), then maybe MenuSearch/MenuSearchInput could be placed inside of that.

This isn't a blocker since it'd require some discussion and doesn't pertain to the original issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcoker @mcarrano

Right now it's matching the css class naming but I agree that Header may be more usefully generic.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I can see an argument for calling this a header where you can place a search input and potentially other things.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I like MenuHeader and also agree with @thatblindgeye below that it can be done in another issue.

thatblindgeye
thatblindgeye approved these changes Apr 5, 2023
View reviewed changes
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.

For the linked issue this looks good. We can always open a followup regarding the convo above.

tlabaj
tlabaj approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

@kmcfaul can you open a follow up issue please. And we can mere this PR

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Apr 10, 2023

Opened #8933 as a follow up for the MenuSearch > MenuHeader discussion

@tlabaj tlabaj merged commit 9b64ccf into patternfly:v5 Apr 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcarrano mcarrano mcarrano left review comments

@srambach srambach srambach left review comments

@wise-king-sullyman wise-king-sullyman wise-king-sullyman approved these changes

@tlabaj tlabaj tlabaj approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

Assignees
No one assigned
Labels
css approved css review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Menu - filter/search naming and markup, props
7 participants
@kmcfaul @patternfly-build @mcarrano @wise-king-sullyman @srambach @tlabaj @thatblindgeye