feat(filter): L3-4114 - create dumb filter component #418
Conversation
Zach-OfAllTrades
requested review from
davidicus,
justinelliottcobb,
adietrich3074,
scottdickerson,
constantinehuzenko,
aahill50 and
sgrund14
as code owners
✅ Deploy Preview for phillips-seldon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
davidicus
changed the title
|
when you click on this part it should trigger checkbox |
|
I think this after click state is kinda weird, maybe we can add some border radius? |
|
the text's height is 8px which kinda cuts it, I'm afraid we will have incorrect spacing because of that |
src/components/Filter/Filter.tsx
Outdated
| <div | ||
| {...commonProps} | ||
| className={classnames(baseClassName, className, { | ||
| [`${px}-has-separators`]: !isLast, |
There was a problem hiding this comment.
not really clear, why do we need this separator?
There was a problem hiding this comment.
okay I see now, maybe we should Separator component for that, @scottdickerson wdyt?
There was a problem hiding this comment.
When there are multiple filters, the design shows there is a separator between them. This Filter component, for the majority of cases I would think, would be implemented alongside other Filters. I created the FilterControl component to wrap and manage these.
|
this one is responsible for showing that button, I guess it should be separated component, @scottdickerson please confirm |
| key={value.label} | ||
| label={value.label} | ||
| onChange={(e) => { | ||
| e; |
There was a problem hiding this comment.
not sure why we need this e here
|
I guess only one radio button can be selected
|
I kept this as the same component because the only difference was number of options and button placement. When implemented inside the FilterControl component, the different states of being viewAll or not are managed and is displayed with a slide transition. |
I noticed this too but figured it would be handled when the logic was implemented but now on second thought I will probably need to pass some sort of isActive prop down... |
I'm not sure we want a "focus" state instead of a "focus-visible" state. I don't think a border should show on an input when it's been clicked. |
|
small alignment issue when I'm viewing All, where the checkboxes don't directly align with the labels
|
| import Input from '../Input/Input'; | ||
|
|
||
| // You'll need to change the ComponentProps<"htmlelementname"> to match the top-level element of your component | ||
| export interface FilterValueProps extends Omit<ComponentProps<'div'>, 'onChange'> { |
There was a problem hiding this comment.
Just an FYI this would be a good use of a Type Generic where the ValueType is passed and used in a value prop. So consumers would say <FilterValue<boolean>> for instance and our prop model would say value: ValueType. We're not doing it in the <Input components either so this is kinda tech debt across seldon
|
|
||
| return ( | ||
| <div {...commonProps} className={classnames(baseClassName, className)} {...props} ref={ref}> | ||
| <form action={action}>{parsedChildren}</form> |
There was a problem hiding this comment.
I need to be able to pass in the <form element from outside this component so I can pass the Remix <Form component. Perhaps following the element pattern we use in other places?
| {...props} | ||
| ref={ref} | ||
| > | ||
| <Text |
There was a problem hiding this comment.
could you use the normal <Input label prop rather than construct your own here?
There was a problem hiding this comment.
Tried that originally but couldn't see where I had control over the placement, it was always right of the input. Could be missing something, though.
|
still can't click at some points |
|
can we add padding bottom? so scroll will go little further
|
| // label, | ||
| inputType = 'checkbox', | ||
| isHidden = false, | ||
| // onChange, |
There was a problem hiding this comment.
let's remove unnecessary props
| /> | ||
| <Input | ||
| {...inputProps} | ||
| // checked={isActive} |
There was a problem hiding this comment.
we should remove this as well
# [1.97.0](v1.96.1...v1.97.0) (2024-11-22) ### Features * **filter:** L3-4114 - create dumb filter component ([#418](#418)) ([0014771](0014771))
|
🎉 This issue has been resolved in version 1.97.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Jira ticket
L3-4114
Screenshots
Patterns._.FilterControl.-.Overview.Storybook.-.Google.Chrome.2024-11-06.10-16-29.mp4
Summary
This creates a dumb filter component but also manages the state between child and parent filters (like when clicking view all) along with the transition between the two. These components are meant to be composable. In storybook, I implemented this filter in a Drawer component as it will be on the sale page.
Change List (describe the changes made to the files)
Acceptance Test (how to verify the PR)
Being that this is a dumb component, the actual filtering logic will need to be written when implemented. The main thing to test here is that the component and its children render correctly, view all appears after 10 filters is exceeded, that all filters are listed when view all is click, a back button is present on the view all, and that there is a transition between the two.
Things to look for during review
feat(scope): ...if aminorrelease should be triggered.phillipsclass prefix are using the prefix variabledata-testidattribute.New Components
index.tsfilecomponentStyles.scssfile.