SelectMultiple Component #6275

jcfilben · 2022-08-12T19:41:12Z

What does this PR do?

Adds the SelectMultiple component.

Where should the reviewer start?

What testing has been done on this PR?

Added storybook stories

How should this be manually tested?

Do Jest tests follow these best practices?

Note: still working on adding unit tests

screen is used for querying.
The correct query is used. (Refer to this list of queries)
userEvent is used in place of fireEvent.
asFragment() is used for snapshot testing.

Any background context you want to provide?

Applicable WCAG Rules for this component:

What are the relevant issues?

Closes #6260

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes

Should this PR be mentioned in the release notes?

Yes

Is this change backwards compatible or is it a breaking change?

Backwards compatible

ericsoderberghp

A few things to start with.

I'm hoping we can try to improve on the complexity currently in Select. For example, requiring value to always be an Array. There's also a few of (...) : (...) when rendering where each part is rather large. I'm wondering if there's a way to simplify by splitting things up a bit. Just thinking out loud at the moment.

src/js/components/MultiSelect/MultiSelect.js

ericsoderberghp · 2022-08-17T00:44:41Z

src/js/components/MultiSelect/MultiSelect.js

+    const optionIndexesInValue = useMemo(() => {
+      const result = [];
+      allOptions.forEach((option, index) => {
+        if (Array.isArray(valuedValue)) {


Can we assume for SelectMultiple that the value is always an Array? I'm wondering if that can help simplify the code.

Yes, done in latest commit

ericsoderberghp · 2022-08-17T00:49:18Z

src/js/components/MultiSelect/MultiSelect.js

+
+    const SelectIcon = getSelectIcon(icon, theme, open);
+
+    const optionLabel = useCallback(


Since we're going to call this function to render every option anyway, I'm wondering if we can skip the useCallback and just call getOptionLabel() directly as needed?

Done in latest commit. Should I do this for getOptionLabel as well?

yep, there were a few in the same pattern

ericsoderberghp · 2022-08-17T00:52:02Z

src/js/components/MultiSelect/MultiSelect.js

+                      valueKey && valueKey.reduce ? applyKey(i, valueKey) : i;
+                    const indexOptions = allOptions.indexOf(i);
+                    const iLabel = optionLabel(indexOptions);
+                    if (value.indexOf(iValue) < 5) {


This is nested pretty deep in here. I'm wondering if there's a way to restructure it to break it up a bit.

I moved some of the code here out into a separate function. (See visibleValue())

ericsoderberghp · 2022-08-17T00:53:57Z

src/js/components/MultiSelect/MultiSelect.js

+                      }
+
+                      return (
+                        <Button


I'm wondering if we can make a more common Button for use in both Select and SelectMultiple? Not the children, but the props.

I changed this button to be SelectOption. There are a few places we use SelectOption so I am currently looking to see if there is enough similarities in them make some props common for SelectOption

Excellent! That's the kind of thing I am hoping we can do more of.

ericsoderberghp · 2022-08-17T00:58:03Z

src/js/components/MultiSelect/MultiSelect.js

+      </Box>
+    );
+
+    const defaultSelectTextInput = (


Do we need to render this always? It seems like we don't always use it.

I changed this in that latest commit

ericsoderberghp · 2022-08-19T04:59:24Z

What about splitting it up into even more discrete components? For example, SelectContainer, SelectValue, SelectClear, SelectOptions, SelectOption. Where each has its own file and properties related solely to its function?

jcfilben · 2022-08-19T17:47:48Z

What about splitting it up into even more discrete components? For example, SelectContainer, SelectValue, SelectClear, SelectOptions, SelectOption. Where each has its own file and properties related solely to its function?

Sounds good I will try something like this out. Marking this PR as a draft in the meantime while I work on this

src/js/components/MultiSelect/MultiSelect.js

taysea

Nice work on this big component! I noticed there's a lot of considerations for accessibility, thank you!

I've dropped some initial comments on naming/some functionality things I noted. I'll probably have more questions as I can dig deeper into the code but these were just some first things.

src/js/components/MultiSelect/MultiSelectContainer.js

src/js/components/MultiSelect/MultiSelectValue.js

src/js/components/MultiSelect/index.d.ts

…is true

MikeKingdom

What does separateSelected do? It isn't obvious to me what effect it has.

MikeKingdom · 2022-08-29T19:46:57Z

src/js/components/SelectMultiple/stories/typescript/Children.tsx

+        background="brand"
+        round="large"
+      >
+        <Text size="small">{season}</Text>


The font color and icon color in HPE theme is not very readable.

Fixed, changed the background color to 'light-3' instead of 'brand'

MikeKingdom · 2022-08-29T19:49:32Z

src/js/components/SelectMultiple/stories/typescript/Children.tsx

+                pad={{ vertical: 'xsmall', horizontal: 'small' }}
+                margin="xsmall"
+              >
+                Select Season


This should probably be text-weak or whatever the placeholder color is?

Good idea, I changed the text to 'text-weak' (same as placeholder)

MikeKingdom · 2022-08-29T19:52:51Z

src/js/components/SelectMultiple/SelectMultipleValue.js

+      </Box>
+      {value && value.length > theme.selectMultiple.visibleInline && (
+        <Box alignSelf="start">
+          <Button


This button looks a bit odd in grommet or base theme since it has no padding between its border and the select's borders

I added some padding around the button

MikeKingdom · 2022-08-29T20:14:30Z

src/js/components/SelectMultiple/stories/typescript/Children.tsx

+    // Uncomment <Grommet> lines when using outside of storybook
+    // <Grommet theme={...}>
+    <Box fill align="center" pad="small">
+      <SelectMultiple


This example looks odd with the 0 of 10 selected text when nothing is selected and wasn't clear, especially with the placeholder being the default text color

I changed this so that when nothing is selected the 0 out of x doesn't show and instead we just see the placeholder or valueLabel

ericsoderberghp · 2022-08-30T06:47:45Z

src/js/components/Select/DefaultSelectTextInput.js

+    ref,
+  ) => (
+    <SelectTextInput
+      a11yTitle={a11yTitle}


Can this just be part of ...rest?

ericsoderberghp · 2022-08-30T06:48:25Z

src/js/components/Select/DefaultSelectTextInput.js

+      defaultCursor={disabled === true || undefined}
+      focusIndicator={false}
+      id={id ? `${id}__input` : undefined}
+      name={name}


Can this just be part of ...rest?

ericsoderberghp · 2022-08-30T06:48:51Z

src/js/components/Select/DefaultSelectTextInput.js

+      {...rest}
+      tabIndex="-1"
+      type="text"
+      placeholder={placeholder}


Can this just be part of ...rest?

ericsoderberghp · 2022-08-30T06:49:20Z

src/js/components/Select/DefaultSelectTextInput.js

+      placeholder={placeholder}
+      plain
+      readOnly
+      value={value}


…eview

taysea · 2022-08-31T19:39:13Z

src/js/components/SelectMultiple/index.d.ts

+
+export interface SelectMultipleProps extends BasicSelectProps {
+  defaultValue?: (string | number | object)[];
+  help?: React.ReactNode;


Can this just be a string of text?

Yes, added string to the type definition

taysea · 2022-08-31T19:42:54Z

src/js/components/SelectMultiple/index.d.ts

+  limit?: number;
+  value?: (string | number | object)[];
+  showSelectedInline?: boolean;
+  separateSelected?: boolean;


separateSelected doesn't immediately make sense to me. Thinking out loud with some ideas:

selectionsTop

sortSelectedOnClose

liftSelected

groupSelected

Also, should the value default to true ? or will that cause issues

I changed the prop name to sortSelectedOnClose and have it default to true

My one concern with using "sort" is that if the caller has ordered the options to be: [A, C, B, E, D] and the user selects [C, B], the name "sortSelectedOnClose" implies to me that the component will covert the selected to [B, C].

taysea · 2022-08-31T19:44:49Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+          }
+        }
+        if (usingKeyboard)
+          setShowA11yLimit('Selected. Maximum selection limit reached.');


This string message seems to read weirdly

Should it just be "Maximum selection limit reached."

I know the wording seems a little strange, just some context: this message gets read when an option is selected and the number of selected options is now equal to the limit. I wanted to give some confirmation that the option was actually selected. I was thinking if the message just read 'Maximum selection limit reached' the user might be confused as to whether their option was selected or not which is why I have it read 'Selected' first.

taysea · 2022-08-31T19:46:00Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+      if (showA11yLimit !== undefined) {
+        setTimeout(() => {
+          setShowA11yLimit(undefined);
+        }, 2000);


Suggested change

}, 2000);

}, 2000); // value chosen based on length of a11yLimit message

taysea · 2022-08-31T19:50:16Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+              flex={false}
+              pad={{ horizontal: 'small', top: 'xsmall' }}
+            >
+              <Box


This whole return block feels a bit repetitive -- should this Box be built into SelectionSummary? Then depending on if showSelectedInline or not, the Box props could be altered. Seems like since SelectionSummary should present in a row, that the box defining the row should be part of it.

let inlineContent = ( <SelectionSummary allOptions={allOptions} clearRef={clearRef} disabled={disabled} disabledKey={disabledKey} isSelected={isSelected} labelKey={labelKey} limit={limit} onChange={onChange} options={options} search={search} setActiveIndex={setActiveIndex} value={value} valueKey={valueKey} boxProps={showSelectedInline ? { pad: { vertical: 'xsmall' }, direction: "row", justify: "between", gap: "small", fill: "horizontal" } : { .... } } /> ); if (showSelectedInline) inlineContent = ( <Box direction="row" justify="between" flex={false} pad={{ horizontal: 'small', top: 'xsmall' }} > {inlineContent} <Button icon={<FormUp />} onClick={onClose} a11yTitle="Close Select" plain /> </Box> ); ... return ( .... <StyledContainer> {inlineContent} ....

Good idea, I broke this out as suggested

taysea · 2022-08-31T19:59:05Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+                  );
+                  const optionSelected = value.includes(iValue);
+                  const optionActive = activeIndex === index;
+                  const iLabel = getOptionLabel(


Suggested change

const iLabel = getOptionLabel(

const optionLabel = getOptionLabel(

feel like this is a little more aligned w grommet conventions

Yes, fixed in latest commit

taysea · 2022-08-31T19:59:25Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+                show={activeIndex !== -1 ? activeIndex : undefined}
+              >
+                {(option, index, optionRef) => {
+                  const iValue =


Suggested change

const iValue =

const optionValue =

feel like this is a little more aligned w grommet conventions

Yes, fixed in latest commit

taysea · 2022-08-31T20:00:42Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+                      iLabel.toLowerCase().indexOf(search) >= 0
+                    ) {
+                      // code to bold search term in matching options
+                      const boldIndex = iLabel.toLowerCase().indexOf(search);


How do I test this functionality?

When you type in the search you should see substrings that match your search term bolded. I realized that this wasn't working on the grommet theme, but fixed this in the latest commit.

taysea · 2022-08-31T20:02:03Z

src/js/components/SelectMultiple/SelectMultipleValue.js

+                            labelKey || valueKey,
+                          )}`,
+                      );
+                    }, 200);


add comment for how this value was chosen?

taysea · 2022-08-31T20:02:49Z

src/js/components/SelectMultiple/SelectMultipleValue.js

+            key={iLabel}
+            id={`selected-${iValue}`}
+          >
+            {children && child ? (


Suggested change

{children && child ? (

{child ? (

Are children required in order for child to receive a value?

Yes, I simplified this in the latest commit

ericsoderberghp · 2022-08-31T22:15:15Z

src/js/components/SelectMultiple/SelectMultiple.js

+const StyledSelectBox = styled(Box)`
+  ${(props) => !props.callerPlain && controlBorderStyle};
+  ${(props) =>
+    props.theme.select &&


With the way ?. works, do we need the intervening checks still? Can this just be ${(props) => props.theme.select?.control?.extend}

Changed to ${(props) => props.theme.select?.control?.extend} in latest commit

ericsoderberghp · 2022-08-31T22:16:36Z

src/js/components/SelectMultiple/SelectMultiple.js

+import { SelectMultiplePropTypes } from './propTypes';
+
+const StyledSelectBox = styled(Box)`
+  ${(props) => !props.callerPlain && controlBorderStyle};


I realize this is inherited from prior Select, but I'm wondering if "plainSelect" might be a bit more descriptive rather than "callerPlain" as a property name.

Changed to plainSelect in latest commit

src/js/components/SelectMultiple/SelectMultiple.js

ericsoderberghp · 2022-08-31T22:23:06Z

src/js/components/SelectMultiple/SelectMultiple.js

+        if (
+          normalizedValue &&
+          normalizedValue.length > 0 &&
+          normalizedValue.some((v) => v === applyKey(option, valueKey))


Can these all be simplified to: normalizedValue?.some(...)?

If I just have normalizedValue?.some(...) I get the error: normalizedValue.some is not a function. I modified this to if (normalizedValue.length && normalizedValue?.some(...))

What about normalizedValue?.some?.(...)

Yeah that works! I'll push up that change

ericsoderberghp · 2022-08-31T22:28:35Z

src/js/components/SelectMultiple/SelectMultiple.js

+    useEffect(() => {
+      if (separateSelected && ((open && search) || !open)) {
+        const selectedOptions = optionsProp.filter((option) => {
+          const iValue =


suggestion: "iValue" -> "optionValue"

src/js/components/SelectMultiple/SelectMultiple.js

…ault and incorporated other feedback from review

ericsoderberghp · 2022-09-01T21:47:05Z

src/js/components/Select/Select.js

-        return optionLabelKey;
-      return undefined;
-    }, [labelKey, allOptions, optionIndexesInValue, selectValue]);
+    const displayLabelKey = useMemo(


As an example of custom hooks, this could be useDisplayLabelKey(labelKey, allOptions, optionIndexesInValue, selectValue). Not required, just illustrating.

Good to know, thanks for the example

ericsoderberghp · 2022-09-01T21:52:06Z

src/js/components/Select/StyledSelect.js

    props.theme.select.container && props.theme.select.container.extend};
 `;
+
+// position relative is so scroll can be managed correctly
+export const OptionsBox = styled.div`


Since this isn't actually a "Box", can we call it something like "OptionsContainer"?

ericsoderberghp · 2022-09-02T15:42:44Z

src/js/components/Select/SelectContainer.js

@@ -294,7 +244,13 @@ const SelectContainer = forwardRef(
        let nextActiveIndex = activeIndex + 1;
        while (
          nextActiveIndex < options.length &&
-          isDisabled(nextActiveIndex)
+          checkDisabled(


Suggest "checkDisabled" -> "isDisabled", to align more with "isSelected"

ericsoderberghp · 2022-09-02T15:47:28Z

src/js/components/Select/utils.js

+export const getOptionValue = (index, options, valueKey) =>
+  applyKey(options[index], valueKey);
+
+export const checkDisabled = (


If you made this a custom hook, it might make its usage cleaner:

const isDisabled = useDisabled(disabled, disabledKey, options, valueKey); ... if (isDisabled(nextActiveIndex) ...

where useDisabled would then simply be:

const useDisabled(disabled, disabledKey, options, valueKey) = useCallback((index) => {...}, [disabled, disabledKey, options, valueKey]);

There's just a lot of lines of code for dependencies in the other useCallbacks() in SelectContainer.js

Changed to use useDisabled hook

ericsoderberghp · 2022-09-02T15:54:10Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+        } else if (optionsRef.current) {
+          setActiveIndex(0);
+        }
+      }, 100);


Can you add a comment indicating why "100" was chosen?

ericsoderberghp · 2022-09-02T15:56:00Z

src/js/components/SelectMultiple/SelectMultipleContainer.js

+
+                  if (!children && search) {
+                    if (
+                      typeof optionLabel === typeof '' &&


Why not typeof optionLabel === 'string'?

Changed to typeof optionLabel === 'string' in latest commit

ericsoderberghp · 2022-09-02T15:59:21Z

src/js/themes/base.d.ts

@@ -1395,6 +1395,9 @@ export interface ThemeType {
    searchInput?: ReactComponentElement<any>;
    step?: number;
  };
+  selectMultiple?: {
+    visibleInline?: number;


Suggest "visibleInline" -> "maxInline" to be a little clearer that this is a maximum. I think "inline" implies "visible".

Good idea, I changed this to maxInline in the latest commit

… feedback

jcfilben added 2 commits August 11, 2022 12:19

working version

bbf85aa

code cleanup

042045b

jcfilben marked this pull request as draft August 12, 2022 19:41

jcfilben added 3 commits August 15, 2022 15:30

code consolidation

b12e48b

addition consolidation and proptypes cleanup

59bf67b

added a simple test

1096bcf

ericsoderberghp reviewed Aug 17, 2022

View reviewed changes

jcfilben marked this pull request as ready for review August 18, 2022 14:30

Changes from review feedback

b5c1353

jcfilben requested review from britt6612 and taysea August 18, 2022 18:13

jcfilben marked this pull request as draft August 19, 2022 17:47