Add SelectSearch #929
Add SelectSearch #929
Conversation
Oksamies
force-pushed
the
add-select-search
branch
from
c00d51c to
9249c9d
Compare
Oksamies
force-pushed
the
add-select-search
branch
from
9249c9d to
2cac4a6
Compare
Oksamies
force-pushed
the
multi-select-search
branch
from
94c0e37 to
c2ff54f
Compare
Oksamies
force-pushed
the
add-select-search
branch
from
2cac4a6 to
79399f6
Compare
Oksamies
force-pushed
the
add-select-search
branch
from
79399f6 to
7fdbd0a
Compare
There was a problem hiding this comment.
Rejecting because our SelectSearch doesn't implement the Search part at all. Should be a quick fix?
Also worth noting that (at least some of) the issues raised on the review of #919 apply here too due to the copy-paste implementation.
| } = useController({ name }); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
Would it make sense to use div here? E.g. with flex the error span can get placed oddly. Just thinking aloud here, not sure if it's necessary.
There was a problem hiding this comment.
I meant replacing this <> fragment with a div, not the <span> inside. Now all the potential problems apply to that <div> as well since it's going to be rendered as a sibling to the <SelectSearch>
| import { isNode } from "../../utils/type_guards"; | ||
|
|
||
| type Props = { | ||
| options: string[]; |
There was a problem hiding this comment.
I think this should use the same kind of "label & value" object as the multiselect, both for consistency and ease-of-use when working with options provided by the backend. Or is there some specific reason for going string-only?
There was a problem hiding this comment.
Went for the string-only, solely for simplicity. Though I'd keep it string-only for now and edit the component, once the label value pair is needed.
There was a problem hiding this comment.
It's your call. My two cents is that the simplicity that is achieved by using just strings seems pretty trivial. The work that is required to redo the interface once the component is in use in multiple places seems like a higher price to pay.
| * Cyberstorm SelectSearch component | ||
| * TODO: Better keyboard navigation | ||
| */ | ||
| export const SelectSearch = React.forwardRef<HTMLInputElement, Props>( |
There was a problem hiding this comment.
Lot of code repetition going on here. Did you consider that one of the selects should extend the other, or both extend some base implementation? I.e. do it under the hood, so that we would still export two different components, with similar (not necessarily identical) props. I'm asking because at a quick glance the only difference is that in single-select the add functions overrides the previous value instead of pushing it to the list.
Sometimes having separate components will be better for keeping the implementations simple and making changes easy, so it's not wrong to do so here. But in this case the similarities are pretty comprehensive, so using a different approach could make sense. "More art than science" kinda thing, really.
There was a problem hiding this comment.
I'd like to keep it as a separate component for now. (so no extending)
The reasoning is that; SelectSearch isn't even up to design specs and is kind of an "a component that works for now", so I don't want to invest too much time into refining this, until we've decided to either improve this or scrap it.
Also in general I've been trying to not make spaghetti and keep everything separated, so it's easier to refactor later. Even if it means we'll have a lot of similar code in different places.
There was a problem hiding this comment.
Again your call, but please don't assume that the only two options on the table are "spaghetti" and "separated". Also properly done reuse can actually make future refactoring easier, not harder. Case in point: fixing the issues in other PR requires remembering to do the changes in both components.
Oksamies
force-pushed
the
add-select-search
branch
2 times, most recently
from
9de4897 to
a7df0b5
Compare
| } = useController({ name }); | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
I meant replacing this <> fragment with a div, not the <span> inside. Now all the potential problems apply to that <div> as well since it's going to be rendered as a sibling to the <SelectSearch>
| import { isNode } from "../../utils/type_guards"; | ||
|
|
||
| type Props = { | ||
| options: string[]; |
There was a problem hiding this comment.
It's your call. My two cents is that the simplicity that is achieved by using just strings seems pretty trivial. The work that is required to redo the interface once the component is in use in multiple places seems like a higher price to pay.
| * Cyberstorm SelectSearch component | ||
| * TODO: Better keyboard navigation | ||
| */ | ||
| export const SelectSearch = React.forwardRef<HTMLInputElement, Props>( |
There was a problem hiding this comment.
Again your call, but please don't assume that the only two options on the table are "spaghetti" and "separated". Also properly done reuse can actually make future refactoring easier, not harder. Case in point: fixing the issues in other PR requires remembering to do the changes in both components.
| > | ||
| {options | ||
| .filter((option) => | ||
| option.toLowerCase().includes(search.toLowerCase()) |
There was a problem hiding this comment.
This will retain the selected value in the dropdown. Pointing it out since it differs from how the MultiSelectSearch works. Is this intentional and how we want these components to work?
There was a problem hiding this comment.
I'd say yes, we want the selected to be there, though an indicator on the selected item might be great. I'll come back to this after merging and rebasing the bigger PR 👍
Oksamies
force-pushed
the
add-select-search
branch
from
72a5dc7 to
035d7d3
Compare
No description provided.