-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/filter component #313
Conversation
056fbf4
to
20e54bb
Compare
50483b9
to
1e5913a
Compare
align-items: center; | ||
justify-content: center; | ||
background-color: rgb(243, 244, 246); | ||
color: rgba(0,0,0); |
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.
flisespikk, men hvorfor ikke bare skrive "black" og spare å kjøre en funksjon? :)
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.
det skal byttes uansett, koster ingenting
<Button size="small" onClick={onBtnClick} disabled={disabled} variant="secondary" color="first"> | ||
<PlusIcon /> {t('filter_bar.add_filter')} | ||
</Button> | ||
{isOpen && ( |
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.
Potentially refactor this into a <ButtonWithDropdown />
-component? To extract the button, isOpen and dropdown functionality.
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.
Vi må se hvor gjenbrukbart det kan bli på sikt, og behovene. Ser ingen verdi i å skille ut dette som egen komponent foreløpig.
* /> | ||
* ``` | ||
*/ | ||
export const FilterBar = ({ onFilterChange, fields, initialFilters = [] }: FilterBarProps) => { |
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.
Maybe we could write a unit test for this component?
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.
PR looks great, missing unit tests (but that could potentially be its own PR).
Venter med enhetstester til denne er blitt litt mer moden og vi er blitt enige om på hvilket nivå vi skal legge oss. |
Dynamisk filterkomponent (
Filterbar
) ansvarlig for visning og kontroll av filtre, som generer et sett med filtre for filtrering av data.Hva er endret?
Dette er første iterasjon.
Dokumentasjon / Storybook / testdekning
Eksempler i Storybook.
stories
iStorybook
, eller så er ikke dette relevant.Skjermbilder eller GIFs (valgfritt)