-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generic selection component #2013
Conversation
14e09e3
to
daec644
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job but a Select component is not only used for filters, but for selects 😄
Could you do the whole composition about the all
options on the outside layer?
826a1b2
to
b1f8931
Compare
@dottorblaster good point thanks! At the end I decided to let the used define whether an |
c9483af
to
1eb3386
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job, just one thing to adjust. This way we could reuse this component as a generic select in the future and not only as a filter. That's why I asked you to move out the whole "all" logic outside of the component to increase the chance for composition.
options={PROVIDERS} | ||
withAllOption |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options={PROVIDERS} | |
withAllOption | |
options={[ALL_OPTION, ...PROVIDERS]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could reuse this component as a generic select in the future and not only as a filter.
We can already do this, even with the withAllOption
prop, right?
…ing is implemented
a52f855
to
40ed6f0
Compare
@dottorblaster heres the last two commits 8e02b2a and 40ed6f0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks!
@dottorblaster in total honesty I was confused by the whole "all" logic outside 😅 Anyway, thanks for the review! |
Description
Generalizes the current
ProviderSelection
to aSelect
which will be used also for other filters in the catalog.How was this tested?
Test and storybook