Add MultiSelectSearch component #919
Conversation
|
Note that this component is being improved in #920 . I'm looking for tips and opinions |
There was a problem hiding this comment.
Most of the stuff is not that important, but I would consider fixing at least the ones that are straightforward and quick to fix.
By far the most important comment is the one regarding the double state management. If there's no need to duplicate it, I would change the implementation. Marking as request changes because of this issue, but I can probably be swayed with good arguments.
apps/cyberstorm-storybook/stories/components/MultiSelectSearch.stories.tsx
Show resolved
Hide resolved
apps/cyberstorm-storybook/stories/components/MultiSelectSearch.stories.tsx
Outdated
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.module.css
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.module.css
Outdated
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.tsx
Outdated
Show resolved
Hide resolved
| </Icon> | ||
| </button> | ||
| </div> | ||
| <div |
There was a problem hiding this comment.
I can't recall how selects normally handle this but it looks a bit awkward if the dropdown is open but filteredOptions show no options to choose from
There was a problem hiding this comment.
Decided to go with showing an empty dropdown instead of hiding it. Found a little less confusing to have an empty list, instead of nothing happening on screen. In the end designs decide though.
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.tsx
Outdated
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.tsx
Outdated
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.tsx
Outdated
Show resolved
Hide resolved
packages/cyberstorm/src/components/MultiSelectSearch/MultiSelectSearch.tsx
Outdated
Show resolved
Hide resolved
Oksamies
force-pushed
the
multi-select-search
branch
from
94c0e37 to
c2ff54f
Compare
| }; | ||
|
|
||
| const Template: StoryFn<typeof MultiSelectSearch> = (args) => { | ||
| const [selected, setSelected] = useState(options); |
There was a problem hiding this comment.
This now sets all options selected initially, so the user has nothing to choose from before they unselect something. Not the best demo case.
There was a problem hiding this comment.
Changed selected to empty list 👍
| <div style={{ color: "white" }}> | ||
| Value in state:{" "} | ||
| {selected | ||
| .map((s) => { |
There was a problem hiding this comment.
Nitpick: .map((s) => s.label)
| return ( | ||
| <div className={styles.root} ref={ref}> | ||
| <div className={styles.selected}> | ||
| {value.map((option) => { |
There was a problem hiding this comment.
Currently the options are listed in the order they are selected. IDK if it would be more user friendly if they were ordered alphabetically? Something to think about e.g. when fixing the other TODOs later on?
| ref={menuRef} | ||
| > | ||
| <div className={styles.inputContainer} data-color={color}> | ||
| <input |
There was a problem hiding this comment.
On Storybook, at least with Firefox, the tab navigation will now get stuck into this input, not moving forward no matter how many times I tab.
There was a problem hiding this comment.
Fixed this issue, but I'll keep the keyboard navigation todo there, because I'm not 100% what is the best practice, so we need to fully fix the keyboard nav later.
| </button> | ||
| <div className={styles.inputButtonDivider}></div> | ||
| <button | ||
| onClick={(e) => { |
There was a problem hiding this comment.
On Storybook, at least with Firefox, when the menu is invisible, I'd expect clicking this would make it visible. But the menu stays visible only for a fraction of a second, then it closes and the focus moves into the input.
There was a problem hiding this comment.
Probably was broken because of the focus fuckery in the last comment you made, should be fixed now 👍
| @@ -3,3 +3,10 @@ export const isRecord = (obj: unknown): obj is Record<string, unknown> => | |||
|
|
|||
| export const isStringArray = (arr: unknown): arr is string[] => | |||
| Array.isArray(arr) && arr.every((s) => typeof s === "string"); | |||
|
|
|||
| export const isNode = (e: EventTarget | null): e is Node => { | |||
| if (!e || !("nodeType" in e)) { | |||
There was a problem hiding this comment.
Nitpick. I suspect you've fallen victim to one of the classic blunders, namely only fixing the line mentioned in a comment and disregarding the context. Now that this no longer throws an error, it would make more sense to be just
export const isNode = (e: EventTarget | null): e is Node =>
e !== null && "nodeType" in e;
Add ordering to selected Half-fix onFocus / Tab event interractions Cleanup isNode refs TS-1968
TS-1968