feat(Select): add examples, add popper interface, fix focus #8992

kmcfaul · 2023-04-24T19:41:10Z

What: Closes #8874

Updates examples in Select:

adds disabled state checkbox to basic example
adds divider to select group

Adds examples to Select:

option variation example
view more example
creatable typeahead example
creatable multitypeahead example
checkbox multitypeahead example

Other changes:

Adds footer example to menu examples.
Adds blurb over props pointing to Menu documentation for additional props, and adds commonly used props directly to the Select props.
Adds an interface that includes commonly used popper props for select and included the interface in the docs.
Fixes a description spelling mistake in menu.
Forwards ref through SelectOption to enable view more

Addressing #8985

Fixes the toggle focus with Esc/Tab - added a preventDefault that was interfering with Tab
Adds toggle ref to the selection callback to allow a user to focus the toggle after a selection (since the toggle ref is not controlled or set by the user). Alternately, if having the callback param isn't desired I could swap to a shouldCloseOnSelect flag that triggers the focusing internally

Notes:

ARIA disabled styling is not present on menu currently, so that variation was not included on the option variation example. I have a local stash adding an isAriaDisabled flag similar to other components that include the prop, but the style modifier doesn't exist on menu.
As the default behavior of Select is to have tab close the menu, the "menu with footer" example was added to the custom menus page instead so the tab behavior can be controlled.

patternfly-build · 2023-04-24T19:50:04Z

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

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

mcarrano

@kmcfaul this is looking good. Just a couple of issues/questions:

One small doc update request.
The "Typeahead with creatable options" does not seem to be working correctly. If I use this to add another state to the menu, it becomes selected in the toggle, but opening the menu again does not show me all states, I need to clear the selection to get this. Once an item is selected, shouldn't is go back to displaying the entire list? Isn't this the way the old example worked?
I see that you added the "With footer" example to the custom menu demos. Why? I think that the desire to have an example is Select was to make sure consumers don't think that feature was dropped.

mcarrano · 2023-04-27T14:46:32Z

packages/react-core/src/components/Select/examples/Select.md

+
+```
+
+### Option variations


Can we call this something more descriptive like, "Menu item variants" or just add some descriptive text to say what this is?

Sure I will update the description.

For the other feedback:

Both single typeaheads behave this way currently, but I can update them to stop filtering after a selection (currently the examples use the input value to do filtering - the filter value just needs to be tracked in a separate state).

I added it there because Select's built-in tab behavior is incompatible with the example (tab needs to have its default behavior rather than close the select). Since this PR I updated Dropdown/Menu to allow customization of those closing keys, so I can add that update here as well and move it back into Select.

The other reason for adding the demo to custom menus was because things like MenuSearch and MenuFooter lay outside the MenuContent, but in Select and Dropdown the MenuContent is internal so the extra children would be under the content instead of outside it. I think this is only a DOM difference though, without affecting keyboard or visual behavior.

packages/react-core/src/components/Select/examples/SelectOptionVariations.tsx

packages/react-core/src/components/Select/Select.tsx

packages/react-core/src/components/Select/examples/SelectMultiTypeaheadCheckbox.tsx

packages/react-core/src/components/Select/examples/SelectViewMore.tsx

packages/react-core/src/demos/ComposableMenu/examples/MenuWithFooter.tsx

nicolethoen · 2023-05-01T20:50:06Z

ARIA disabled styling is not present on menu currently, so that variation was not included on the option variation example. I have a local stash adding an isAriaDisabled flag similar to other components that include the prop, but the style modifier doesn't exist on menu.

Sounds like this should be a post v5 core issue? Could you open one?

kmcfaul · 2023-05-03T18:07:59Z

@nicolethoen Opened patternfly/patternfly#5528 to track the aria-disabled styling.

thatblindgeye

Recent updates look good, just had two other things below

packages/react-core/src/components/Select/Select.tsx

packages/react-core/src/components/Select/examples/SelectFooter.tsx

mcarrano

@kmcfaul this looks good. Just one other small ask. Can we change the style of the button in the Footer example to a plain link-style button? While it's not really relevant from a coding standpoint, I don't think we'd want to use a primary button like this in a menu footer and people do take these examples very literal. So it should look like the below. Thanks!

mcarrano

Looks great. Thanks @kmcfaul !

tlabaj

Something seems to have regressed with the checkbox select spacing.
cc @mcoker

tlabaj

A general comment for the select examples. I think the should be updated to to use the internally generated menuRef ref rather than the consumer passing a ref. I don't want the consumers to think the have to pass the ref.

tlabaj · 2023-05-15T22:09:42Z

packages/react-core/src/components/Select/Select.tsx

+  onSelect?: (
+    event?: React.MouseEvent<Element, MouseEvent>,
+    itemId?: string | number,
+    toggleRef?: React.RefObject<any>


So the whole purpose of having the toggle render is so that consumer does not have to pass a ref. I think we should not expect the consumer to use the ref in this way. I would opt for the shouldCloseOnSelect prop instead.

The ref looks like it was a holdover from when we manually wired everything up and wasn't currently being used so I removed them.

I added a shouldFocusToggleOnSelect prop (the ref is only for that purpose as opening/closing is entirely user-side). However, the "view more" example will break with this change because it should not be focusing the toggle when the selection is the loading option, so I've left the optional property in for now and added comments to the specific example and description of the callback. Will that work?

Or alternately we could add an optional toggleRef parameter (similar to how ref is optional and without anything passed it will default to an internal ref) and let toggle accept a node as well as a function to allow a user to control the toggleRef completely. What do you think?

@thatblindgeye @nicolethoen which approach do you think is more intuitive? I m leaning towards the first approach but i can see benefits of the second as well.

I'd agree that either approach could work. I might lean more towards the toggleRef prop approach personally as it offers that flexibility and isn't tied to just onSelect being called, but considering we're mainly trying to focus the toggle "on select" I'd be fine with the PR as it is.

Just a couple of things:

Will this require Dropdown to be updated similarly?

For the prop description on onSelect and shouldFocusToggleOnSelect, maybe adding in some verbiage to explain that one or the other should be used and not both.

Pushed an update with the alternate method, which I can back out if necessary. I kind of like the fallback being manual control of the toggleRef instead of the callback because then a user can control focus in other circumstances as well, not just selection. The view more example showcases the new method.

Also updated Dropdown similarly.

tlabaj · 2023-05-16T15:40:12Z

Something seems to have regressed with the checkbox select spacing. cc @mcoker

@kmcfaul @mcoker I am opened a issue #9128 against the React menu for this one. They issue doe not see, to exist in core.

tlabaj · 2023-05-17T19:24:50Z

packages/react-core/src/components/Select/Select.tsx

@@ -13,13 +32,19 @@ export interface SelectProps extends MenuProps, OUIAProps {
  isOpen?: boolean;
  /** Single itemId for single select menus, or array of itemIds for multi select. You can also specify isSelected on the SelectOption. */
  selected?: any | any[];
-  /** Renderer for a custom select toggle. Forwards a ref to the toggle. */
-  toggle: (toggleRef: React.RefObject<any>) => React.ReactNode;
+  /** Select toggle. May be a direct ReactNode combined with the toggleRef property, or a renderer for a custom select toggle which forwards a ref to the toggle. */


The description is not quite right. Aren't both option a custom toggle? I think we want to be explicit that if you do not use a renderer, you need to provide a ref. I wonder if we should group the props somehow. Maybe an object for the react node toggle and the ref. then the toggle prop could be either that or the renderer?

Since there's no default I thought that "custom" was a bit redundant to state. For the required ref, I tried to convey that in May be a direct ReactNode combined with the toggleRef property [...] but let me try to reword that better.

@tlabaj Do the descriptions make more sense now?

packages/react-core/src/components/Dropdown/Dropdown.tsx

tlabaj

Thanks for the updates! LGTM

packages/react-core/src/components/Select/Select.tsx

thatblindgeye

Also have the same question as Nicole above

packages/react-core/src/components/Dropdown/Dropdown.tsx

patternfly-build · 2023-05-18T14:25:49Z

Your changes have been released in:

@patternfly/react-code-editor@5.0.0-alpha.109
@patternfly/react-core@5.0.0-alpha.108
@patternfly/react-docs@6.0.0-alpha.116
demo-app-ts@5.0.0-alpha.92
@patternfly/react-table@5.0.0-alpha.110

Thanks for your contribution! 🎉

@gildub

…ecated `DropdownItems`, use `isAriaDisabled` prop for items with tooltips (#1150) Fixes a regression caused by #1126 where dropdown items that are disabled with tooltips could not trigger the tooltips. Discovered by @gildub when working on #1149. It appears the `isAriaDisabled` prop which we need for retaining the mouse hover event on a disabled DropdownItem is not present in the new PF5 DropdownItem. This is due to missing styles in PF core that are still being addressed (see patternfly/patternfly#5528, patternfly/patternfly#5692, and related comment here: patternfly/patternfly-react#8992 (comment)) Until `isAriaDisabled` is supported in the new PF5 DropdownItem we must restore the usage of the deprecated component for these tooltips to work.

…ly#8992) * feat(Select): add examples, add popper interface, fix focus * feedback round 1 * fix component reference in docs * remove unneeded toggle focus * remove popperprops extension, add beta to keys * remove container * update snap for removal of container div * update button * remove unneeded ref, add focus flag & comments * toggleRef option * update dropdown examples * snap * update desc * update wording * use single prop * move toggle object to interface, add to docs * prop req and remove extra prop

nicolethoen requested review from nicolethoen, mcarrano, tlabaj and andrew-ronaldson April 27, 2023 12:07

nicolethoen added ux review accessibility labels Apr 27, 2023

nicolethoen requested a review from thatblindgeye April 27, 2023 12:08

mcarrano requested changes Apr 27, 2023

View reviewed changes

kmcfaul mentioned this pull request Apr 27, 2023

feat(Dropdown): fix focusing, add popper props #9005

Merged

thatblindgeye requested changes Apr 27, 2023

View reviewed changes

This was referenced May 1, 2023

chore(Dropdown): updated deprecated usage to new dropdown #8868

Merged

PrimaryDetail demos - update deprecated Select with new Select #9018

Closed

kmcfaul force-pushed the select-next-examples branch from 647d5e2 to 6664793 Compare May 3, 2023 17:56

nicolethoen approved these changes May 3, 2023

View reviewed changes

thatblindgeye requested changes May 4, 2023

View reviewed changes

packages/react-core/src/components/Select/Select.tsx Outdated Show resolved Hide resolved

packages/react-core/src/components/Select/examples/SelectFooter.tsx Outdated Show resolved Hide resolved

kmcfaul force-pushed the select-next-examples branch from d80720d to 7821279 Compare May 8, 2023 15:00

thatblindgeye approved these changes May 9, 2023

View reviewed changes

kmcfaul force-pushed the select-next-examples branch from a9dccb3 to 37e259f Compare May 9, 2023 15:03

nicolethoen requested a review from mcarrano May 9, 2023 20:38

mcarrano requested changes May 10, 2023

View reviewed changes

kmcfaul force-pushed the select-next-examples branch from 7383df1 to 58aeae5 Compare May 11, 2023 17:24

nicolethoen requested a review from mcarrano May 11, 2023 20:02

mcarrano approved these changes May 11, 2023

View reviewed changes

tlabaj reviewed May 15, 2023

View reviewed changes

tlabaj requested changes May 15, 2023

View reviewed changes

kmcfaul force-pushed the select-next-examples branch from 58aeae5 to 884ced5 Compare May 16, 2023 14:28

tlabaj requested changes May 17, 2023

View reviewed changes

kmcfaul force-pushed the select-next-examples branch from c02ceb3 to 644cbaf Compare May 17, 2023 20:38

tlabaj reviewed May 17, 2023

View reviewed changes

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved

kmcfaul added 14 commits May 17, 2023 17:29

feat(Select): add examples, add popper interface, fix focus

bbdea8a

feedback round 1

5f5a25f

fix component reference in docs

627b006

remove unneeded toggle focus

cd30db1

remove popperprops extension, add beta to keys

3ed427f

remove container

2a986e5

update snap for removal of container div

1cc4afc

update button

e0013fc

remove unneeded ref, add focus flag & comments

4a92893

toggleRef option

e08da3f

update dropdown examples

a355c75

snap

0f4aeba

update desc

052f7c2

update wording

8ab9226

kmcfaul force-pushed the select-next-examples branch from 644cbaf to 8ab9226 Compare May 17, 2023 21:31

kmcfaul added 2 commits May 17, 2023 17:51

use single prop

dbdeded

move toggle object to interface, add to docs

1badf52

tlabaj approved these changes May 18, 2023

View reviewed changes

nicolethoen reviewed May 18, 2023

View reviewed changes

packages/react-core/src/components/Select/Select.tsx Outdated Show resolved Hide resolved

thatblindgeye requested changes May 18, 2023

View reviewed changes

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved

prop req and remove extra prop

47a46f0

thatblindgeye approved these changes May 18, 2023

View reviewed changes

thatblindgeye merged commit 4b296dd into patternfly:v5 May 18, 2023

mturley mentioned this pull request Jul 17, 2023

🐛 Restore deprecated Dropdown in KebabDropdown, use matching deprecated DropdownItems, use isAriaDisabled prop for items with tooltips konveyor/tackle2-ui#1150

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Select): add examples, add popper interface, fix focus #8992

feat(Select): add examples, add popper interface, fix focus #8992

kmcfaul commented Apr 24, 2023 •

edited

Loading

patternfly-build commented Apr 24, 2023 •

edited

Loading

mcarrano left a comment

mcarrano Apr 27, 2023

kmcfaul May 3, 2023

kmcfaul May 3, 2023

nicolethoen commented May 1, 2023

kmcfaul commented May 3, 2023

thatblindgeye left a comment

mcarrano left a comment

mcarrano left a comment

tlabaj left a comment

tlabaj left a comment

tlabaj May 15, 2023

kmcfaul May 16, 2023

tlabaj May 17, 2023

thatblindgeye May 17, 2023

kmcfaul May 17, 2023 •

edited

Loading

tlabaj commented May 16, 2023 •

edited

Loading

tlabaj May 17, 2023

kmcfaul May 17, 2023

kmcfaul May 17, 2023

tlabaj left a comment

thatblindgeye left a comment

patternfly-build commented May 18, 2023


		```

		### Option variations

feat(Select): add examples, add popper interface, fix focus #8992

feat(Select): add examples, add popper interface, fix focus #8992

Conversation

kmcfaul commented Apr 24, 2023 • edited Loading

patternfly-build commented Apr 24, 2023 • edited Loading

mcarrano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolethoen commented May 1, 2023

kmcfaul commented May 3, 2023

thatblindgeye left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcfaul May 17, 2023 • edited Loading

Choose a reason for hiding this comment

tlabaj commented May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlabaj left a comment

Choose a reason for hiding this comment

thatblindgeye left a comment

Choose a reason for hiding this comment

patternfly-build commented May 18, 2023

kmcfaul commented Apr 24, 2023 •

edited

Loading

patternfly-build commented Apr 24, 2023 •

edited

Loading

kmcfaul May 17, 2023 •

edited

Loading

tlabaj commented May 16, 2023 •

edited

Loading