Feature/search multiselect #117
Conversation
✅ Deploy Preview for gumdrops ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
||
| ## SearchMultiSelectExtended Props | ||
|
|
||
| <ArgsTable of={SearchMultiSelectExtended} /> |
There was a problem hiding this comment.
it says ArgsTable is not found.. need to import it as well as Canvas and Source (see others for example)
There was a problem hiding this comment.
@belinda82 it's giving an error that SearchMultiSelectExtendedStories is not found (https://deploy-preview-117--gumdrops.netlify.app/?path=/docs/molecules-searchmultiselectextended--docs)
There was a problem hiding this comment.
This component can display hierarchical data or nested options, also has tags to know which options you have chosen, and you can search for the value
|
do we want to have a separate component for this or merge its functionality into the multiselect component? @morganee thoughts? |
|
@belinda82 what is the difference between this and the other one? i'm not clear. thanks! |
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
22fe73c to
4408cd5
Compare
|
@mnicole Hey hi.. I changed the description and the name of the component to "SearchMultiSelectGroups" as describes better the goal of this component. Let me know ur thoughts. |
|
thanks @belinda82 i was wondering if grouping should be the default behavior on the regular SearchMultiSelect? instead of having 2 different components with replicated code, I think we should just be able to use SearchMultiSelect and depending on the format of the data you pass in, it will either group it or not (similar to MultiSelect where it supports grouping but we don't have a separate MultiSelectGroup.. it will automatically group it if you pass in nested data) |
|
Hi @mnicole I merged both components into one single component. let me know your thoughts. |
| </div> | ||
| <div className={tagHolderClasses}> | ||
| <div className="gds-search-select__tag-overflow"> | ||
| {options.map(({ name, key, isSelected, options }, index) => { |
There was a problem hiding this comment.
it looks like it's still working but just wanted to make sure.. this PR is changing the structure of the options you pass in to have "value" instead of "key".. will this still work if someone had already been passing in "key"? and it works still if you pass in "value" since key is referenced here?
can you try these steps https://github.com/gumgum/gumdrops/blob/master/CONTRIBUTING.md#local-testing to test it on AM in a place where it's using the options using key, and one using value?
also it's a little hard to test on gumdrops since you have to edit the options manually so would be good to test on a real app!
There was a problem hiding this comment.
I test it and it works, also did some changes to improve performance
| case 13: // enter | ||
| this._toggleOnEnter(); | ||
| break; | ||
| case 27: // esc |
There was a problem hiding this comment.
is there a reason to remove these other keyboard shortcuts?
|
@morganee can you help review this one too since it's a lot of changes? 🥺 |
belinda82
force-pushed
the
feature/searchMultiselect
branch
2 times, most recently
from
8dddaa0 to
fd319dc
Compare
|
@morganee the component is ready to test :D thanks!! |
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
fd319dc to
1832e0b
Compare
| <Checkbox | ||
| key={i} | ||
| value={value} | ||
| onChange={() => _toggleOption(option)} |
There was a problem hiding this comment.
To select an option we have to click on the checkbox only, before it seems we could by clicking anywhere in the row. Is it possible to move back the _toggleOption(option) (and I think toggleSubItem(option, subIndex) too) on the parent div instead of the Checkbox ?
There was a problem hiding this comment.
good call .. thanks Morgane 👯 , its done!
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
1832e0b to
f1985d5
Compare
|
For the nested options, the first option in the doc 'All pets' is not showing unless it has options. Maybe we could make it explicit that if they pass the isNested prop then we read only values with options. If it's not passed, so the default option, we just display the top level values, whether they have options or not. If we need to have a mix of both in the future we can update the component but I don't think it's a use case that needs to be addressed right away. Moreover they if they have a nested array with orphan values, they can put them under a fake parent that they call 'Other', 'Unknown' etc, it's not a blocker. |
|
talked to Belinda on video but i think we should show the high level value by default if they pass one in. it could be the case in the hierarchy that they want to know if someone selects a general category but that general category maybe doesn't have any sub categories. like you said Morgane, in the future if needed we could make it so they can pass isNested and that determines it but it might be an edge case. |
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
f1985d5 to
4f98c57
Compare
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
4f98c57 to
b3cad37
Compare
|
Approved for me ! Belinda showed me the latest code on her local. She needs to rotate her github ssh keys to get the access back to push the doc changes. Also I don't see an approval button, hence the comment. |
|
i just found a button that says "re-request a review" 🤷♀️ |
yes thank you I see a button now to review the changes. I'll wait for Belinda to push to approve 👍 |
belinda82
force-pushed
the
feature/searchMultiselect
branch
from
b3cad37 to
eab1254
Compare
|
@morganee changes done 👯 |
No description provided.