feat(DualListSelector): add new component #5120

nicolethoen · 2020-11-11T15:06:53Z

What: Closes #5019

patternfly-build · 2020-11-11T15:12:44Z

PF4 preview: https://patternfly-react-pr-5120.surge.sh
A11y report: https://patternfly-react-pr-5120-a11y.surge.sh

mcarrano

Great start @nicolethoen . Just a couple comments:

Arrow buttons should be disabled when there is nothing selected to move.
In the second example, the text above the list boxes ("# available" or "#chosen") does not seem right. Is there a reason this should be different than for the first example?

@mmenestr would be good if you can also review.

nicolethoen · 2020-11-11T20:00:19Z

@mcarrano In the second example I was just demonstrating that the default status can be overwritten and consumer defined. By default it behaves like the first example

codecov-io · 2020-11-11T20:52:40Z

Codecov Report

Merging #5120 (aba9a5f) into master (198dd0f) will decrease coverage by 0.31%.
The diff coverage is 30.71%.

@@            Coverage Diff             @@
##           master    #5120      +/-   ##
==========================================
- Coverage   52.59%   52.28%   -0.32%     
==========================================
  Files         568      571       +3     
  Lines       10474    10627     +153     
  Branches     3917     3958      +41     
==========================================
+ Hits         5509     5556      +47     
- Misses       4263     4366     +103     
- Partials      702      705       +3

Flag	Coverage Δ
patternfly4	`52.28% <30.71%> (-0.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...c/components/DualListSelector/DualListSelector.tsx	`17.89% <17.89%> (ø)`
...mponents/DualListSelector/DualListSelectorPane.tsx	`50.00% <50.00%> (ø)`
...ents/DualListSelector/DualListSelectorListItem.tsx	`62.50% <62.50%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5191590...aba9a5f. Read the comment docs.

evwilkin · 2020-11-11T21:48:23Z

@nicolethoen there are 2 things in the examples that are a bit confusing to me - first, when deselecting an item, the background color remains on the item until you click away (the text color does change from blue to black as expected):

Also - in the second example, it looks like the count for the "Chosen options" is mirroring the state for the "Available options" instead of actually giving the count for the chosen options:

evwilkin · 2020-11-11T21:49:39Z

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

+        availableOptionsStatus={`${this.state.availableOptions.length} available`}
+        availableOptionsActions={availableOptionsActions}
+        chosenOptions={this.state.chosenOptions}
+        chosenOptionsStatus={`${this.state.availableOptions.length} chosen`}


Suggested change

chosenOptionsStatus={`${this.state.availableOptions.length} chosen`}

chosenOptionsStatus={`${this.state.chosenOptions.length} chosen`}

nicolethoen · 2020-11-11T21:51:08Z

@evwilkin
nice catch on the second note. that's my bad.
as far as the background color - I think that's because the button still has focus after you deselect it. so the grey background is a result of the focus being on the item even though it's been deselected.
@mcoker do you know if that's an implementation problem on my end? or is that a css issue as far you can tell?

mcarrano · 2020-11-11T22:32:34Z

@mcarrano In the second example I was just demonstrating that the default status can be overwritten and consumer defined. By default it behaves like the first example

Can we note/document that as part of the example then? In looking at this, not sure I'd understand why the text is different. Maybe even just rename the example "Custom header with filter" or something like that?

evwilkin · 2020-11-12T16:37:45Z

@nicolethoen it looks like there's a bug in the keyboard interaction, I can't select either of the < or << options with the keyboard unless one of the "Available options" is selected. The buttons will appear active as expected and can be clicked, but cannot be accessed via keyboard.

In the top example below I can't navigate down inside the controls panel, and in the bottom example I can only navigate down to the first control (>>) unless I select an "Available option", then I can use the keyboard to navigate between all available controls.

mmenestr · 2020-11-12T16:44:39Z

To add to the discussion on the customizable text underneath the filters, I feel like we should keep them the same for both examples. Even if they are customizable, we want to encourage people to stick with the same wording of "X out of X selected", so I would keep it the same throughout the examples.

It goes back to the debate of how much freedom/customizability we want to give people and I don't think that should be one that we should "encourage" customizability for. I can add some text in the design guidelines i'm drafting up for this to say that it's customizable but I'd still recommend keeping it as designed!

The other thing is that the filters don't seem to work - is that as intended?

tlabaj · 2020-11-12T16:50:50Z

I agree with Evan. It is a confusing to me that the the item that is not selected but is hovered on has the same background color as a selected item.

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

packages/react-core/src/components/DualListSelector/DualListSelectorPane.tsx

nicolethoen · 2020-11-12T16:54:12Z

@jessiehuff Tell me how you think this sounds:

As I understand it, the relationship between the html structure as written in core and the expectations of the VO tools is a little misaligned.

the VO tools are expecting role='listbox' to be tracking focus on its role='option' children which in the case of a list, should be the ul and li elements. But our list has buttons within the li and the buttons are what are getting the focus when the user uses the keyboard to navigate.

i propose we open a core issue to do two things:

consider removing the buttons in each li so we can add the event handlers to the li and move focus around the list items like this example
add the appropriate aria attributes to the core examples so we can be sure that the html structure matches what the VO tools expect. This way the relationship between html and VO expectations will be better defined before adding js interactions.

mcoker · 2020-11-12T19:57:43Z

@nicolethoen @evwilkin @mcarrano

when deselecting an item, the background color remains on the item until you click away (the text color does change from blue to black as expected)
do you know if that's an implementation problem on my end? or is that a css issue as far you can tell?

Yep that's a CSS issue because the element is still focused after you click on it, even when you've deselected it. Maybe we should update to remove the focus styles or make them unique?

mcoker · 2020-11-12T20:05:38Z

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

+  /** Actions to be displayed above the available options pane. */
+  availableOptionsActions?: React.ReactNode[];
+  /** Title applied to the chosen options pane. */
+  chosenOptionsTitle?: string;


Do we want to have a default for this or should it be required by the user to give the panes a name?

the default for this is set on line 78. It defaults to "Chosen options" but the user can override it.

Oh sorry, I was asking if we should just make this required instead of providing a default title. I wasn't expecting the title to come with the component by default, though maybe it's fine. wdyt @mcarrano, if you don't pass a title to either the available or chosen panes, should we provide this title by default?

Yes, I think that's fine.

mcoker · 2020-11-12T20:10:38Z

packages/react-core/src/components/DualListSelector/DualListSelectorPane.tsx

+                  return (
+                    <DualListSelectorListItem
+                      key={index}
+                      isSelected={selectedOptions.indexOf(index) !== -1}


This should go on the button (<DualListSelectorItem>)

mcoker

Looks really good! The selected class just needs to go on the item <button>, not the parent <li>. That will also make the selected item bold.

jessiehuff

This is seriously awesome, Nicole!!!! Really nice job with this so far, this one is a complicated one. The keyboard interaction is mostly good, but I noticed one odd behavior. Once all of the options are to the right, I'm having trouble accessing the arrow buttons. If I were to choose all the options and then change my mind or notice that one of the options weren't right, I wouldn't be able to move it back over to the available options section:

Also, I'm still struggling to use this with VO at all. It doesn't announce when items are selected (always says "not selected"), and I can't seem to perform any of the actions. I suspect it's the same issue that we've been talking about with the structure of the li and buttons. We may need to determine what the right structure and aria attributes are needed in react and then update core so that we can ensure that it's the right interaction. As long as we're able to potentially change the structure in the future, I'm ok with this going in and following up with another issue if you're unable to fix it here.

…e examples auto sort

evwilkin · 2020-11-16T19:40:48Z

@nicolethoen the fixes look good but I caught one keyboard bug - if your search returns a subset of the options, it doesn't look like there's a way to keyboard in to select any of the options. Note that if I try a search that returns all options (such as searching the word 'Option') it appears to work just fine.

evwilkin · 2020-11-16T20:57:20Z

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

+  };
+
+  componentDidMount() {
+    window.addEventListener('keydown', this.handleKeys);


This is killing all arrow key functionality for me as it calls event.preventDefault() on line 259 above for any arrow key. Even outside of the component, this is disabling the arrow keys - maybe @jessiehuff knows a better way to attach this listener

oh you're right, the preventDefault is in the wrong place :)

kmcfaul · 2020-11-17T01:43:31Z

I'm noticing now that clicking the sort button the first time doesn't sort the options. If you toggle it a second time, it sorts, but is backwards to what it should be as indicated by the button. Moving options between the lists does sort them now, and will 'fix' the sort order if its backwards, but then hitting the sort button again after that will again not sort the options.

It seems like the code thinks the list is already sorted properly for the first sort button press?

nicolethoen · 2020-11-17T02:59:11Z

It seems like the code thinks the list is already sorted properly for the first sort button press?

@kmcfaul Do you think I should default the list to be unsorted by default? I think the icons are facing the correct direction based on their names.
if you click it while it is a 'descending icon' it makes the items descend highest to lowest.
It may be a little confusing on the first click because it sorts them ascending (and the icon is the ascending icon) but by default the list is already ascending.

kmcfaul · 2020-11-17T03:18:48Z

Not sure how to clip a video, so here's what I was doing to see this behavior:

From a fresh page, clicking the sort icon on the Available pane doesn't change the order. If you move any items out to the Chosen pane, or move all options to Chosen and back, the lists' sorting gets updated. If you click the Available sort button again, it again doesn't change the order. If you click the button a second time, the order is changed, but it is backwards from what the icon indicates. If you move the options, the sorting is updated.

I think it could be this line:
if (prevState.availableDescending) returnValue = returnValue * -1; in the onSort function.
Changing prevState.availableDescending to !prevState.availableDescending appears to fix it for me (and the same change in the chosen pane counterpart).

nicolethoen · 2020-11-17T04:10:18Z

I simplified it. I removed the changing asc/desc icon since it wasnt clear if clicking an ascending icon would make sort it in ascending order, or if it was already in ascending order.

Now in the beginning the list is not sorted so that clicking the button clearly sorts it in ascending order. And as you move items around, the items are not automatically sorted, but you can click the button to sort them in ascending order.

is this approach better?

evwilkin

LGTM!

kmcfaul

LGTM the example sorts now 👍

mcarrano

Looks good! Thanks @nicolethoen .

jessiehuff

LGTM! Really awesome job with this, Nicole!! 🏆 🎉

I created an issue for the follow up screen reader work. 🙂

nicolethoen · 2020-11-17T18:18:28Z

LGTM! Really awesome job with this, Nicole!! 🏆 🎉

I created an issue for the follow up screen reader work. 🙂

@jessiehuff ah i meant to do that, thank you!!

nicolethoen requested review from mcarrano, jessiehuff, mcoker, tlabaj and kmcfaul November 11, 2020 15:06

tlabaj added css review ux review labels Nov 11, 2020

mcarrano requested changes Nov 11, 2020

View reviewed changes

evwilkin reviewed Nov 11, 2020

View reviewed changes

nicolethoen force-pushed the add_dual_list_selector branch from 3a5e368 to 00afa25 Compare November 12, 2020 14:18

tlabaj added the accessibility label Nov 12, 2020

kmcfaul reviewed Nov 12, 2020

View reviewed changes

mcoker reviewed Nov 12, 2020

View reviewed changes

mcoker requested changes Nov 12, 2020

View reviewed changes

nicolethoen force-pushed the add_dual_list_selector branch from 4517f86 to 5edb294 Compare November 12, 2020 23:21

jessiehuff reviewed Nov 13, 2020

View reviewed changes

nicolethoen force-pushed the add_dual_list_selector branch 2 times, most recently from 8970c9b to a778d0d Compare November 13, 2020 19:10

nicolethoen added 5 commits November 16, 2020 13:49

move selected modifier to button

1a77a3f

fix more keyboard issues

00ef1cc

fix another discovered bug

fd41aed

add catch all onListChange callback for simpler list management

c1b7a08

add integration demo and cypress tests, add isSearchable example, mak…

d6c676a

…e examples auto sort

nicolethoen dismissed stale reviews from mcoker, mcarrano, and evwilkin via d6c676a November 16, 2020 18:50

nicolethoen force-pushed the add_dual_list_selector branch from 1000885 to d6c676a Compare November 16, 2020 18:50

evwilkin reviewed Nov 16, 2020

View reviewed changes

nicolethoen added 5 commits November 16, 2020 16:20

fix how i build optionsRefs array so searching actually works

bf6f94b

make sure event.preventDefault called at correct time

aba9a5f

update how we build optionsRef again...

ef54cc4

now i'm just leaving typos...

4986915

update snapshots

cba0a92

simplify sorting in last example

a4caccb

tlabaj approved these changes Nov 17, 2020

View reviewed changes

evwilkin approved these changes Nov 17, 2020

View reviewed changes

kmcfaul approved these changes Nov 17, 2020

View reviewed changes

mcarrano approved these changes Nov 17, 2020

View reviewed changes

jessiehuff approved these changes Nov 17, 2020

View reviewed changes

mcoker approved these changes Nov 17, 2020

View reviewed changes

evwilkin merged commit 0ed1c5a into patternfly:master Nov 17, 2020

nicolethoen deleted the add_dual_list_selector branch February 8, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DualListSelector): add new component #5120

feat(DualListSelector): add new component #5120

nicolethoen commented Nov 11, 2020

patternfly-build commented Nov 11, 2020 •

edited

Loading

mcarrano left a comment

nicolethoen commented Nov 11, 2020

codecov-io commented Nov 11, 2020 •

edited

Loading

evwilkin commented Nov 11, 2020

evwilkin Nov 11, 2020

nicolethoen commented Nov 11, 2020

mcarrano commented Nov 11, 2020

evwilkin commented Nov 12, 2020

mmenestr commented Nov 12, 2020 •

edited

Loading

tlabaj commented Nov 12, 2020

nicolethoen commented Nov 12, 2020

mcoker commented Nov 12, 2020

mcoker Nov 12, 2020

nicolethoen Nov 12, 2020

mcoker Nov 12, 2020

mcarrano Nov 12, 2020

mcoker Nov 12, 2020

mcoker left a comment •

edited

Loading

jessiehuff left a comment

evwilkin commented Nov 16, 2020

evwilkin Nov 16, 2020 •

edited

Loading

nicolethoen Nov 16, 2020

kmcfaul commented Nov 17, 2020

nicolethoen commented Nov 17, 2020

kmcfaul commented Nov 17, 2020 •

edited

Loading

nicolethoen commented Nov 17, 2020

evwilkin left a comment

kmcfaul left a comment

mcarrano left a comment

jessiehuff left a comment

nicolethoen commented Nov 17, 2020

	chosenOptionsStatus={`${this.state.availableOptions.length} chosen`}
	chosenOptionsStatus={`${this.state.chosenOptions.length} chosen`}

feat(DualListSelector): add new component #5120

feat(DualListSelector): add new component #5120

Conversation

nicolethoen commented Nov 11, 2020

patternfly-build commented Nov 11, 2020 • edited Loading

mcarrano left a comment

Choose a reason for hiding this comment

nicolethoen commented Nov 11, 2020

codecov-io commented Nov 11, 2020 • edited Loading

Codecov Report

evwilkin commented Nov 11, 2020

evwilkin Nov 11, 2020

Choose a reason for hiding this comment

nicolethoen commented Nov 11, 2020

mcarrano commented Nov 11, 2020

evwilkin commented Nov 12, 2020

mmenestr commented Nov 12, 2020 • edited Loading

tlabaj commented Nov 12, 2020

nicolethoen commented Nov 12, 2020

mcoker commented Nov 12, 2020

mcoker Nov 12, 2020

Choose a reason for hiding this comment

nicolethoen Nov 12, 2020

Choose a reason for hiding this comment

mcoker Nov 12, 2020

Choose a reason for hiding this comment

mcarrano Nov 12, 2020

Choose a reason for hiding this comment

mcoker Nov 12, 2020

Choose a reason for hiding this comment

mcoker left a comment • edited Loading

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

evwilkin commented Nov 16, 2020

evwilkin Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

nicolethoen Nov 16, 2020

Choose a reason for hiding this comment

kmcfaul commented Nov 17, 2020

nicolethoen commented Nov 17, 2020

kmcfaul commented Nov 17, 2020 • edited Loading

nicolethoen commented Nov 17, 2020

evwilkin left a comment

Choose a reason for hiding this comment

kmcfaul left a comment

Choose a reason for hiding this comment

mcarrano left a comment

Choose a reason for hiding this comment

jessiehuff left a comment

Choose a reason for hiding this comment

nicolethoen commented Nov 17, 2020

patternfly-build commented Nov 11, 2020 •

edited

Loading

codecov-io commented Nov 11, 2020 •

edited

Loading

mmenestr commented Nov 12, 2020 •

edited

Loading

mcoker left a comment •

edited

Loading

evwilkin Nov 16, 2020 •

edited

Loading

kmcfaul commented Nov 17, 2020 •

edited

Loading