-
Notifications
You must be signed in to change notification settings - Fork 357
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(Select): add next version using menu components #8115
Conversation
Preview: https://patternfly-react-pr-8115.surge.sh A11y report: https://patternfly-react-pr-8115-a11y.surge.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great Katie! I have a few comments.
packages/react-core/src/next/components/Select/examples/SelectTypeahead.tsx
Show resolved
Hide resolved
packages/react-core/src/next/components/Select/examples/SelectMultiTypeahead.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the comments below, if it's something that could be added into this PR it'd be worth adding some logic to place focus back onto the toggle when an item is selected. Currently selecting an item doesn't provide an visual focus.
packages/react-core/src/next/components/Select/SelectOption.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Select/SelectOption.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Select/examples/SelectCheckbox.tsx
Outdated
Show resolved
Hide resolved
9ec9112
to
1fb42ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These Select updates are so exciting @kmcfaul !
Just a quick question - I'm noticing this on a few of the new react-next
demos... some of them have the Select menu width constrained to 200px rather than taking up the full width of their container! Was this intentional?
Yes, the non typeahead selects would try to fit the toggle content otherwise, which would be too narrow for the menu when opened and also would change size when something got selected (for the selects that update the toggle text). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Other than my previous comment regarding focus after selection (not really a blocker and could be lumped into the followup for the Space key as a general a11y issue), the only other thing that would be good to implement in a followup would be making it so that the the items within the Select menu were given a role of option
(like how the current Select does) so that aria-selected
can be applied. What do you think @kmcfaul?
Have to echo Jenny's excitement about this Next Select!
Yeah I think that's a good idea. I'll add that to the follow up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the typeahead examples more similar to the examples in the standard Select using a list of states for the options? I'm also noticing that these work differently with regards to what happens when you select a value. The old type ahead clears the field after selecting a value and restores the placeholder text. the Next version keeps the partial string that I typed in the field. I feel like the old behavior is what I expect. @mmenestr do you agree?
Also, the "Multiple single" example does not exist on the original Select and I question where this would be used vs the checkbox select. Don't they do the same thing?
This looks correct to me! The only thing I wonder is whether we should show a badge inside the toggle on the selects where a user can select more than one item to match the regular React demos |
@mcarrano @mmenestr Updated the typeahead examples to have the state data set, the multiple selects to show a badge when there are selections, and also updated the typeahead behavior to better match the original select example. |
@kmcfaul I'm looking at how this is presented in the design guidelines and there is a reference to a multi-select as in the Multiple-single example. However this suggests use of the chips as in a typeahead to differentiate it from the checkbox select. Not sure we ever exactly had a code example for that. Can we create that example using the new approach? @mmenestr interested to hear your perspective. How should we better align the React examples with our design guidance? Also, don't seem to be seeing the other changes you pushed when I preview this @kmcfaul . |
@mcarrano @mmenestr @kmcfaul When I suggested adding a multi select. I thought it would match the example in the current select component where we have chips of the selected items. Here https://patternfly-react-pr-8115.surge.sh/components/select#multiple |
label, | ||
...props | ||
}: SelectGroupProps) => ( | ||
<MenuGroup className={css(className)} label={label} {...props}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way for the user to specify the heading level for the group title or other props for the group? Or is that level of customization a use case for building your own menu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it is possible, as all of these select components extend the menu components. In this case the labelHeadingLevel
prop from MenuGroup would be used to set the heading level.
Perhaps the larger question is how we want to organize these extended props. I'm not a huge fan of duplicating the whole prop list into each Select component in case anything gets updated in Menu later on. Maybe in the docs I can pull in the Menu props as well as the Select props? @tlabaj
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying locally, it doesn't find the Menu components during tsDocgen because the next components are in a separate folder (so it's trying to parse the Menu props as menu-next files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmcfaul maybe we should open an issue in org so we can pull in props from src directory into next.
AS far as exposing props for Menu. I think if it is a common use case we can show it in the Select's prop table. If it is something we are not really encouraging/ is not a common use case I would vote to not add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can open a quick follow up to go over the Menu props and see which ones we want to include for the dropdown/select wrappers & add them
packages/react-core/src/next/components/Select/examples/SelectGrouped.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Select/examples/Select.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/next/components/Select/examples/SelectTypeahead.tsx
Show resolved
Hide resolved
@tlabaj @mmenestr Removed the multiple example. I am not sure how my thought process arrived at that combination of factors for a multiple select in retrospect, maybe I was already working on the multiple typeahead when I was asked to add the multiple example and thought it was for something different. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
One thing I noticed is we're generating an empty id
on menu group titles if titleId
(optional) isn't set - is that something we should fix? I can open another issue if so
<HeadingLevel className={css(styles.menuGroupTitle)} id={titleId}> |
Let's open a follow up for the MenuGroup titleId if you agree @tlabaj . I can open a separate PR just to not keep this one held up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as long as we open the follow up issue.
Opened #8162 for the group id issue |
What: Closes #7321
Adds select-next, a select component that utilizes the newer menu components.
Adds a handful of basic select demos to start with as well.