#338 Changes in Picker component related to enabling the multiselect option #357

marcinsawicki · 2022-07-26T09:00:02Z

Resolves #338

Description

After releasing the Picker component as a new approach after Select, we agreed that this component should be configurable to enable the multiselect feature.
That requires some changes in the component logic, to unify the "singleselect" and "multiselect" mode.

Added multiselect prop to enable this option
Added possibility to pass own class name
Changed the styles of components, enabling the 100% width, to make it easier to fit with own design

Storybook

Default

sgraczyk · 2022-07-28T08:45:23Z

packages/react-components/src/components/Icon/Icon.module.scss

+  svg {
+    pointer-events: none;
+  }


Can you elaborate on this one?

To prevent the list open when the user is clicking on the selected item tag icon (multipicker mode), we need to check the event target. This prop will prevent the event handler to see svg as the target object, where we cannot pass the custom data- attribute.

sgraczyk · 2022-07-28T08:48:00Z

packages/react-components/src/components/Picker/Picker.module.scss

+    margin-bottom: 4px;
+    margin-right: 4px;


How about this? Or we want to avoid declaring 0 margin for remaining directions?

Suggested change

margin-bottom: 4px;

margin-right: 4px;

margin: 0 4px 4px 0;

sgraczyk · 2022-07-28T08:51:43Z

packages/react-components/src/components/Picker/Picker.spec.tsx

@@ -7,24 +7,31 @@ import { IPickerProps, Picker } from './Picker';
 // eslint-disable-next-line @typescript-eslint/no-empty-function


I would expect some test that would make sure that a single-select is able to pick only one value (eg. by clicking two different options and making sure only the last one is triggered in onSelect. Another idea is making sure that unselect is working.

In general I believe we could come up with more testing cases.

sgraczyk · 2022-07-28T08:52:55Z

packages/react-components/src/components/Picker/Picker.stories.tsx

+  options: [
+    { key: 'groupA', name: 'Group A title header', groupHeader: true },
+    { key: 'one', name: 'Option one' },
+    { key: 'two', name: 'Option two' },
+    { key: 'three', name: 'Option three' },
+    { key: 'groupB', name: 'Group B title header', groupHeader: true },
+    { key: 'four', name: 'Option four' },
+    { key: 'five', name: 'Option five' },
+    { key: 'six', name: 'Option six', disabled: true },
+    { key: 'seven', name: 'Option seven', disabled: true },
+  ],


Have you considered moving to a common variable, or do we think it won't work well in Storybook docs?

sgraczyk · 2022-07-28T08:54:10Z

packages/react-components/src/components/Picker/Picker.tsx

  size?: TriggerSize;
  placeholder?: string;
  isRequired?: boolean;
  noSearchResultText?: string;
+  multiselect?: boolean;


I'd avoid the boolean type in favor of type like mode/type with values of single | multi.

Suggested change

multiselect?: boolean;

type?: 'single' | 'multi';

sgraczyk · 2022-07-28T08:54:35Z

packages/react-components/src/components/Picker/Picker.tsx

  disabled?: boolean;
  label?: string;
  error?: string;
  options: IPickerListItem[];
-  selectedOption?: IPickerListItem;
+  selectedOptions?: IPickerListItem[];


maybe selected is enough for the name of the prop?

sgraczyk · 2022-07-28T08:58:44Z

packages/react-components/src/components/Picker/Picker.tsx

-    onSelect(item);
+
+    const itemsToSelect = items.filter(
+      (item) => !item.disabled && !item.groupHeader && item.key !== 'select-all'


Consider placing select-all in a variable

Additionally, I'd think about a helper function isItemSelectable that would move this condition outside filter

packages/react-components/src/components/Picker/Picker.tsx

sgraczyk · 2022-07-28T09:01:19Z

packages/react-components/src/components/Picker/Picker.tsx

+  const getSelectedItems = (items: IPickerListItem[]) => {
+    if (!multiselect) {
+      return <div>{items[0].name}</div>;
+    }
+
+    return (
+      <div>
+        {items.map((item) => {
+          return (
+            <Tag
+              key={item.name}
+              className={styles[`${baseClass}__tag`]}
+              dismissible
+              onRemove={() => handleItemRemove(item)}
+            >
+              {item.name}
+            </Tag>
+          );
+        })}
+      </div>
+    );
+  };


It's more of a component than an internal helper function. Consider extracting.

packages/react-components/src/components/Picker/Picker.tsx

sgraczyk · 2022-07-28T09:06:00Z

packages/react-components/src/components/Picker/PickerList.tsx

+              )}
+              onClick={handleOnSelectAllClick}
+            >
+              Select all


Could be a const

Additionally, I believe developers should have control over this text. For example if used in non-English environment

sgraczyk · 2022-07-28T09:09:40Z

packages/react-components/src/components/Picker/Trigger.tsx

  size?: TriggerSize;
-  onClick: () => void;
+  onClick: (e: React.MouseEvent | KeyboardEvent) => void;


I'm not sure if it's a proper approach to handle both mouse and keyboard events using the click name. Either consider onTrigger or handle two separate events, as it's misleading.

packages/react-components/src/components/Tag/Tag.tsx

packages/react-components/src/components/Picker/Picker.spec.tsx

kstvsko

When you chose something and the picker is opened you should see the chosen values in the picker field, not the placeholder.

When you chose something the icon that clears the picker should look differently.

3. On the default state there is something weird near the picker icon.

4. When you select something and then deselect it and stop focusing on the picker the icon to clear the picker stays visible and there is no placeholder text.

sgraczyk · 2022-08-02T13:57:50Z

packages/react-components/src/components/Picker/Picker.tsx

@@ -70,32 +77,72 @@ export const Picker: React.FC<IPickerProps> = ({
    }
  }, [isListOpen]);

-  const handleOnTriggerClick = () => {
-    if (disabled) {
+  const handleOnTrigger = (e: React.MouseEvent | KeyboardEvent) => {


Suggested change

const handleOnTrigger = (e: React.MouseEvent | KeyboardEvent) => {

const handleTrigger = (e: React.MouseEvent | KeyboardEvent) => {

sgraczyk · 2022-08-02T13:58:14Z

packages/react-components/src/components/Picker/Picker.tsx

      return;
    }

-    return setIsListOpen((prev) => !prev);
+    setIsListOpen((prev) => !prev);
  };

  const handleOnClose = () => {
    setIsListOpen(false);
  };

  const handleOnSelect = (item: IPickerListItem) => {


Suggested change

const handleOnSelect = (item: IPickerListItem) => {

const handleSelect = (item: IPickerListItem) => {

sgraczyk · 2022-08-02T14:01:27Z

packages/react-components/src/components/Picker/Picker.tsx

  };

-  const handleOnClearClick = () => {
+  const handleOnClear = () => {


Suggested change

const handleOnClear = () => {

const handleClear = () => {

sgraczyk · 2022-08-02T14:02:37Z

packages/react-components/src/components/Picker/Picker.tsx

          onFilter={handleOnFilter}
        >
-          {selectedItem ? selectedItem.name : placeholder}
+          {selected ? (


It will trigger also for empty array, is this correct here?

Good catch. After we removed the initial state of selected items, we could expect that an empty array can be passed. I will add an additional check here.

sgraczyk · 2022-08-02T14:04:42Z

packages/react-components/src/components/Picker/Trigger.spec.tsx

@@ -84,7 +84,7 @@ describe('<Trigger> component', () => {
    const { getByTestId } = renderComponent({
      ...defaultProps,
      isItemSelected: true,
-      onClearClick,
+      onClear: onClearClick,


Is this expected?

sgraczyk · 2022-08-02T14:04:52Z

packages/react-components/src/components/Picker/Trigger.spec.tsx

@@ -72,7 +72,7 @@ describe('<Trigger> component', () => {
    const onClick = vi.fn();
    const { getByText } = renderComponent({
      ...defaultProps,
-      onClick,
+      onTrigger: onClick,


Is this expected?

mreszke · 2022-08-02T14:33:23Z

packages/react-components/src/components/Picker/TriggerBody.tsx

+  type,
+  onItemRemove,
+}) => {
+  if (type === 'single') {


isn't better to have 2 separate components, one for single picker and second one for multiple picker?

sgraczyk · 2022-09-23T12:11:49Z

packages/react-components/src/components/Picker/Picker.tsx

+            type={type}
+            onItemRemove={handleItemRemove}
+            onFilter={handleOnFilter}
+          />
        </Trigger>
        <PickerList
          selectedItemsKeys={selected ? getSelectedItemsKeys(selected) : null}


I think we can move this whole condition inside a memoized array selectedItemKeys. You can use useMemo here.

sgraczyk · 2022-09-23T12:15:34Z

packages/react-components/src/components/Picker/Picker.tsx

+  };
+
+  const isItemSelectable = (item: IPickerListItem) =>
+    !item.disabled && !item.groupHeader && item.key !== 'select-all';


Consider having 'select-all' in a shared constant

sgraczyk · 2022-09-23T12:18:26Z

packages/react-components/src/components/Picker/Picker.tsx

+  const getSelectedItemsKeys = (items: IPickerListItem[]) =>
+    items ? items.map((item) => item.key) : null;


Consider the following approach

Suggested change

const getSelectedItemsKeys = (items: IPickerListItem[]) =>

items ? items.map((item) => item.key) : null;

const selectedItemsKeys = useMemo(() => {

if (!selected || !items) {

return null;

}

return items.map((item) => item.key);

}, [selected, items]);

jpyzio123 · 2022-10-28T09:54:01Z

Picker with multiselect set to true has different height CSS than picker with it set to false. Non-multiselect picker has specific height values depending on size property, while multiselect picker has the height set to auto
https://user-images.githubusercontent.com/46003125/198558578-f423ad95-7552-41d0-9d87-6600d0e236b5.mp4
Option to Select all is still visible and clickable with multiselect set to false, I think it shouldn't be.
https://user-images.githubusercontent.com/46003125/198559669-a206b8bc-733a-412e-8997-42a2bec0a90c.mp4

jpyzio123 · 2022-11-08T13:27:10Z

in type: multi, the option selectAllOption set to False is still visible and possible to add as regular option

marcinsawicki and others added 3 commits July 22, 2022 09:01

temp commit to move changes

0dbf409

multiselect mode with tags, select all option, unit tests, css changes

65a7802

merge with v1, last fixes

4825a7c

marcinsawicki added the feature New feature or request label Jul 26, 2022

marcinsawicki linked an issue Jul 26, 2022 that may be closed by this pull request

[Picker] Enable multi-select #338

Closed

marcinsawicki added the blocked Work is blocked label Jul 26, 2022

fix for selecting logic and tests

01482c7

marcinsawicki removed the blocked Work is blocked label Jul 26, 2022

revert package-lock

c5385fe

marcinsawicki changed the title ~~#338 Changes related to enabling the multiselect option~~ #338 Changes in Picker component related to enabling the multiselect option Jul 26, 2022

merge with v1

3015269

sgraczyk requested review from TomaszRatajczyk, MichalPaszowski, kstvsko, martagranisz and a team July 28, 2022 08:45

sgraczyk reviewed Jul 28, 2022

View reviewed changes

packages/react-components/src/components/Picker/Picker.tsx Show resolved Hide resolved

sgraczyk reviewed Jul 28, 2022

View reviewed changes

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

sgraczyk reviewed Jul 28, 2022

View reviewed changes

packages/react-components/src/components/Tag/Tag.tsx Show resolved Hide resolved

mreszke reviewed Jul 28, 2022

View reviewed changes

packages/react-components/src/components/Picker/Picker.spec.tsx Show resolved Hide resolved

kstvsko suggested changes Aug 1, 2022

View reviewed changes

TomaszRatajczyk approved these changes Aug 2, 2022

View reviewed changes

sgraczyk reviewed Aug 2, 2022

View reviewed changes

sgraczyk approved these changes Aug 2, 2022

View reviewed changes

mreszke reviewed Aug 2, 2022

View reviewed changes

marcinsawicki added 3 commits August 12, 2022 08:15

changes after review, bugs fixes, code refactor

5be758f

changes

f0a0366

more tests for Picker components

4902477

sgraczyk reviewed Sep 23, 2022

View reviewed changes

sgraczyk approved these changes Sep 23, 2022

View reviewed changes

marcinsawicki added 3 commits September 27, 2022 12:18

fixes after design review

e3d96da

changes after review

de7a252

bug fix related to selecting items in multipicker mode

a239069

kstvsko approved these changes Sep 27, 2022

View reviewed changes

marcinsawicki added 2 commits November 7, 2022 15:50

changes after testing

589f517

fixed issue with select all option

3acbccc

marcinsawicki requested a review from jpyzio123 November 7, 2022 15:00

change logic related to the select all option

d025f3f

jpyzio123 approved these changes Nov 10, 2022

View reviewed changes

marcinsawicki merged commit 75cca89 into v1 Nov 10, 2022

marcinsawicki deleted the DS-173 branch November 10, 2022 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#338 Changes in Picker component related to enabling the multiselect option #357

#338 Changes in Picker component related to enabling the multiselect option #357

marcinsawicki commented Jul 26, 2022 •

edited

Loading

sgraczyk Jul 28, 2022

marcinsawicki Jul 29, 2022 •

edited

Loading

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022 •

edited

Loading

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

sgraczyk Jul 28, 2022

kstvsko left a comment

sgraczyk Aug 2, 2022

sgraczyk Aug 2, 2022

sgraczyk Aug 2, 2022

sgraczyk Aug 2, 2022

marcinsawicki Aug 3, 2022

sgraczyk Aug 2, 2022

sgraczyk Aug 2, 2022

mreszke Aug 2, 2022

sgraczyk Sep 23, 2022

sgraczyk Sep 23, 2022

sgraczyk Sep 23, 2022

jpyzio123 commented Oct 28, 2022

jpyzio123 commented Nov 8, 2022

		@@ -7,24 +7,31 @@ import { IPickerProps, Picker } from './Picker';
		// eslint-disable-next-line @typescript-eslint/no-empty-function

	const handleOnTrigger = (e: React.MouseEvent \| KeyboardEvent) => {
	const handleTrigger = (e: React.MouseEvent \| KeyboardEvent) => {

	const handleOnSelect = (item: IPickerListItem) => {
	const handleSelect = (item: IPickerListItem) => {

		const getSelectedItemsKeys = (items: IPickerListItem[]) =>
		items ? items.map((item) => item.key) : null;

-  const getSelectedItemsKeys = (items: IPickerListItem[]) =>
-    items ? items.map((item) => item.key) : null;
+  const selectedItemsKeys = useMemo(() => {
+    if (!selected || !items) {
+      return null;
+    }
+    return items.map((item) => item.key);
+  }, [selected, items]);

#338 Changes in Picker component related to enabling the multiselect option #357

#338 Changes in Picker component related to enabling the multiselect option #357

Conversation

marcinsawicki commented Jul 26, 2022 • edited Loading

Description

Storybook

Choose a reason for hiding this comment

marcinsawicki Jul 29, 2022 • edited Loading

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

sgraczyk Jul 28, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstvsko 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpyzio123 commented Oct 28, 2022

jpyzio123 commented Nov 8, 2022

marcinsawicki commented Jul 26, 2022 •

edited

Loading

marcinsawicki Jul 29, 2022 •

edited

Loading

sgraczyk Jul 28, 2022 •

edited

Loading