feat(DualListSelector): add a Dual List Selector component #395

Ron-Lavi · 2018-06-10T13:39:28Z

Add DualListSelector component with storybook and tests. fix #379

Storybook
Design

ohadlevy · 2018-06-20T05:47:49Z

@LaViro can you please rebase? thanks

jeff-phillips-18 · 2018-06-20T12:47:56Z

@patternfly/patternfly-react-ux Can someone self-assign to do the UX review please?

Rohoover · 2018-06-20T14:01:29Z

Adding my comments from slack to here for everyone's visibility:

I'm seeing some alignment issues at the top with the filter input colliding with the top of the pane. Additionally the select all checkbox is slightly lower than expected. Other than that it looks and functions very well. Nice work!

patternfly-build · 2018-06-25T15:33:54Z

Deploy preview for patternfly-react ready!

Built with commit 5ae4a54

https://deploy-preview-395--patternfly-react.netlify.com

dmiller9911 · 2018-06-28T04:49:26Z

packages/patternfly-react/src/components/DualListSelector/DualList.test.js

+import DualList from './DualList';
+import mockLists from './mockLists';
+
+test('patternfly Icon is working', () => {


This test does not cover anything other than the initial render. The snapshot is also ~450 lines. Anything over ~30 lines tends to not be that useful since people just skip looking at them.

It would be better to test the actual logic of the component (sorting, filtering, moving, etc).

dmiller9911 · 2018-06-28T04:51:15Z

packages/patternfly-react/src/components/DualListSelector/components/DualListBody.js

+      items={items}
+      filterResults={filterResults}
+      filterTerm={filterTerm}
+      onChange={e => onItemChange(e)}


This can be written as onChange={onItemChange}

dmiller9911 · 2018-06-28T04:52:07Z

packages/patternfly-react/src/components/DualListSelector/components/DualListBody.js

+    />
+  ) : (
+    <div className="pf-dual-list-no-items">
+      {filterResults ? 'No items were found' : 'No items to show'}


these strings should be able to be passed in from props. That will enable localization/translation.

dmiller9911 · 2018-06-28T04:53:29Z

packages/patternfly-react/src/components/DualListSelector/components/DualListCounter.js

+
+const DualListCounter = ({ selected, total }) => (
+  <strong>
+    {selected} of {total} items selected


same with this. maybe make it something like props.getSelectedMessage(selected, total)

dmiller9911 · 2018-06-28T04:54:44Z

packages/patternfly-react/src/components/DualListSelector/components/DualListItems.js

+      <input
+        type="checkbox"
+        name={item.name}
+        onChange={e => onChange(e)}


onChange={onChange}

dmiller9911 · 2018-06-28T04:54:56Z

packages/patternfly-react/src/components/DualListSelector/components/DualListFilter.js

+const DualListFilter = ({ onChange, side }) => (
+  <span className="pf-dual-list-filter">
+    <input
+      onChange={e => onChange(e)}


onChange={onChange}

dmiller9911 · 2018-06-28T05:01:04Z

packages/patternfly-react/src/components/DualListSelector/DualList.js

+  }
+}
+
+DualList.propTypes = {


It looks like currently, you can't do anything with the list values? It is all contained in local state. This needs to either be controlled by having the consumer manage the lists through callbacks and values or an onChange needs to be available. Controlled is a more work, but usually gives more freedom to the consumer.

Ron-Lavi · 2018-08-13T11:42:45Z

@Rohoover thanks !
I have fixed the issues you've mentioned,
Do you see more UX issues? thanks

Rohoover · 2018-08-16T14:37:38Z

@LaViro
One thing I noticed, is that when I use filter you've cleverly more boldly the letters that match in the results, however it's difficult to see because the text in the boxes is already bold. My suggestion is either make the matching letters even more bold, or perhaps a highlight on the letters. Thoughts?

Ron-Lavi · 2018-08-19T11:03:43Z

@Rohoover
Does font-weight: 400 to the list item is enough ?

Ron-Lavi · 2018-08-20T11:17:15Z

@Rohoover I saw that in Foreman they use titles in the selectors:

Should we add that option?

Rohoover

@LaViro
The new bolded type looks better, thank you.

As for the titles... I'm not sure we have room...
I like the titles while at the same time want to stick with the original design.

Maybe @mcarrano would have an opinion.

mcarrano · 2018-08-20T18:11:35Z

I agree with @Rohoover . Not sure that we have room nor do I think they add that much clarity, for example "All items" could be confusing if the list on the left is only a subset of items that are available to this task.

Rohoover · 2018-08-21T14:07:44Z

@mcarrano
Another thought I had... any title could be added above the dual list selector panes. In other words, not having the titles doesn't prohibit a development team from adding it to the page in context to the selectors.

jeff-phillips-18 · 2018-09-04T12:01:11Z

Should this component be added to the patternfly-react-extensions package instead?

ibolton336 · 2018-12-04T18:32:26Z

packages/patternfly-3/patternfly-react/src/components/DualListSelector/DualList.js

+      const isParent = itemsCopy[index].children;
+      if (isParent) {
+        // update the item's children check state.
+        itemsCopy[index].children.forEach(child => {


Can we abstract some of this logic out into testable functions?

Could it wait for a follow up PR?
I think we should let users play with this component and get more feedback.
as I see it, the DualList is kind of an helper to DualListControlled as it receives all of data in a clean way.

I agree adding helpers to Dual List could make things more clear,
though this means breaking many things right now and testing many use cases again.

cdcabrera

First up, appreciate the effort, it does show! much appreciated! But feel a re-evaluation around carrying props over to related components needs to take place.

Reasons:

quite a few repeated props
- this does come across as an effort to avoid having to update multiple components, that is a valid reason, however...
there are also misaligned props, for example onChange vs onItemChange, see comments

Currently a few of these components simply reference other components' props. That can work in some cases, but...

Possible solutions:

It may be a little clearer if the props are either:

redeclared in their respective components, granted that is a bit of extra around updating
or the props all come from a single source instead of a multitude of components
or another possibility is to look at passing the props down, instead of pulling them from a child. This could happen through a generic ...props.
- This also happens to work if the intention is to have a child only ever used internally

...ges/patternfly-3/patternfly-react/src/components/DualListSelector/components/DualListItem.js

.../patternfly-3/patternfly-react/src/components/DualListSelector/components/DualListHeading.js

...es/patternfly-3/patternfly-react/src/components/DualListSelector/components/DualListItems.js

...ges/patternfly-3/patternfly-react/src/components/DualListSelector/components/DualListBody.js

Ron-Lavi · 2018-12-09T10:20:13Z

Thanks @cdcabrera !!
Going to look over duplicated props.
as for the onChange and onItemChange,
the onChange is a callback returned to the consumer when items have been moved between lists.
the onItemChange is when the list item select state is toggled.

patternfly-build · 2018-12-09T16:07:35Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

dtaylor113

@LaViro addressed my review comments, so LGTM, but understand there may be other review comments which may need to be addressed -thanks

patternfly-build · 2018-12-10T16:49:08Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

cdcabrera

Just a few basics around questions, and some minor syntax clean up. Overall, after this, looking good for me

packages/patternfly-3/patternfly-react/src/components/DualListSelector/DualList.js

patternfly-build · 2018-12-11T13:42:29Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

patternfly-build · 2018-12-11T14:53:53Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

Ron-Lavi · 2018-12-11T15:41:06Z

Hey guys, fixed all of your suggestions,
but... found some bugs on filters functionality, please don't review yet.

thanks

patternfly-build · 2018-12-12T16:23:23Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

patternfly-build · 2018-12-13T13:53:47Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

patternfly-build · 2018-12-13T15:20:40Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

Ron-Lavi · 2018-12-13T15:45:29Z

Hey guys, feel free to review now :)

patternfly-build · 2018-12-13T15:57:35Z

PatternFly-React preview: https://395-pr-patternfly-react-patternfly.surge.sh

cdcabrera

This has been a ton of effort @LaViro and it shows kudos!

Just a few notes for a potential round two... looking forward to seeing some actions responses on/through storybook, and some expanded unit tests on those helpers. But going to defer to someone else on those aspects

cdcabrera · 2018-12-13T16:28:57Z

packages/patternfly-3/patternfly-react/src/components/DualListSelector/helpers.js

+  sortBy: 'label',
+  filterTerm: '',
+  isMainChecked: false
+});


note:
Default props in 2 locations, understand why it was done, but it's an indicator for a potential fail/frustration point for anyone trying to update this component.

At minimum a note or reference to this location needs to be called out possibly in the DualList.js file around DualList.defaultProps = {.

Others, of us, may want this to just be a reference to default props contained within the DualList class with something like DualList.defaultProps.left, I'll leave that up to you

packages/patternfly-3/patternfly-react/src/components/DualListSelector/helpers.js

Ron-Lavi · 2018-12-13T17:24:11Z

Pleasure to work with you guys !
Great follow ups suggestions :) :)

Ron-Lavi force-pushed the feature/dual-list-selector branch 3 times, most recently from b15e46c to 3cf32ac Compare June 13, 2018 15:38

Ron-Lavi force-pushed the feature/dual-list-selector branch from 3cf32ac to 7cac4ed Compare June 20, 2018 09:18

Ron-Lavi self-assigned this Jun 20, 2018

Ron-Lavi added css review labels Jun 20, 2018

Ron-Lavi changed the title ~~[W.I.P] feat(DualListSelector): add a Dual List Selector component~~ feat(DualListSelector): add a Dual List Selector component Jun 20, 2018

Ron-Lavi force-pushed the feature/dual-list-selector branch from 7cac4ed to 5ae4a54 Compare June 20, 2018 09:37

Rohoover self-assigned this Jun 20, 2018

dmiller9911 previously requested changes Jun 28, 2018

View reviewed changes

Ron-Lavi force-pushed the feature/dual-list-selector branch from 587d2a4 to 1c46f33 Compare August 14, 2018 08:28

Rohoover reviewed Aug 20, 2018

View reviewed changes

Ron-Lavi force-pushed the feature/dual-list-selector branch 4 times, most recently from 3b2d915 to b4faec7 Compare September 2, 2018 19:57

This comment has been minimized.

Sign in to view

Ron-Lavi force-pushed the feature/dual-list-selector branch from b4faec7 to 2ca97d4 Compare September 2, 2018 20:13

ibolton336 reviewed Dec 4, 2018

View reviewed changes

cdcabrera reviewed Dec 6, 2018

View reviewed changes

Ron-Lavi force-pushed the feature/dual-list-selector branch from fc02a58 to e02768d Compare December 9, 2018 15:51

Ron-Lavi force-pushed the feature/dual-list-selector branch from e02768d to c06ada8 Compare December 10, 2018 16:34

dtaylor113 previously approved these changes Dec 10, 2018

View reviewed changes

cdcabrera reviewed Dec 10, 2018

View reviewed changes

Ron-Lavi dismissed dtaylor113’s stale review via a6ebfa8 December 11, 2018 13:26

Ron-Lavi force-pushed the feature/dual-list-selector branch from c06ada8 to a6ebfa8 Compare December 11, 2018 13:26

Ron-Lavi force-pushed the feature/dual-list-selector branch from a6ebfa8 to a06f862 Compare December 11, 2018 14:37

Ron-Lavi force-pushed the feature/dual-list-selector branch from a06f862 to 0593c6f Compare December 12, 2018 16:05

Ron-Lavi force-pushed the feature/dual-list-selector branch from 0593c6f to fcce2fe Compare December 13, 2018 13:38

Ron-Lavi force-pushed the feature/dual-list-selector branch from fcce2fe to ebfa606 Compare December 13, 2018 15:02

Add DualList component

16dcf34

Ron-Lavi force-pushed the feature/dual-list-selector branch from ebfa606 to 16dcf34 Compare December 13, 2018 15:42

dtaylor113 approved these changes Dec 13, 2018

View reviewed changes

cdcabrera approved these changes Dec 13, 2018

View reviewed changes

jeff-phillips-18 approved these changes Dec 13, 2018

View reviewed changes

jeff-phillips-18 merged commit 8c3447f into patternfly:master Dec 13, 2018

feat(DualListSelector): add a Dual List Selector component #395

feat(DualListSelector): add a Dual List Selector component #395

Uh oh!

Conversation

Ron-Lavi commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ohadlevy commented Jun 20, 2018

Uh oh!

jeff-phillips-18 commented Jun 20, 2018

Uh oh!

Rohoover commented Jun 20, 2018

Uh oh!

patternfly-build commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ron-Lavi commented Aug 13, 2018

Uh oh!

Rohoover commented Aug 16, 2018

Uh oh!

Ron-Lavi commented Aug 19, 2018

Uh oh!

Ron-Lavi commented Aug 20, 2018

Uh oh!

Rohoover left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano commented Aug 20, 2018

Uh oh!

Rohoover commented Aug 21, 2018

Uh oh!

This comment has been minimized.

jeff-phillips-18 commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Reasons:

Possible solutions:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ron-Lavi commented Dec 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Dec 9, 2018

Uh oh!

dtaylor113 left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Dec 10, 2018

Uh oh!

cdcabrera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patternfly-build commented Dec 11, 2018

Uh oh!

patternfly-build commented Dec 11, 2018

Ron-Lavi commented Jun 10, 2018 •

edited

Loading

cdcabrera left a comment •

edited

Loading

Ron-Lavi commented Dec 9, 2018 •

edited

Loading

cdcabrera left a comment •

edited

Loading