refactor(frontend): Define reusable components for search filters, tweak appearance #344
Conversation
amatissart
force-pushed
the
filters-ui-components
branch
from
18fd779
to
9b495ab
Compare
amatissart
changed the title
There was a problem hiding this comment.
The new design is great!
I found one issue related to the security, and one error in the browser console. Can you confirm if you have the same error?
I didn't take the time to spot where the error was triggered, but in the console, moving a slider display this message:
Material-UI: A component is changing the default value state of an uncontrolled Slider after being initialized. To suppress this warning opt to use a controlled Slider.
I confirm that, even if the message considers itself as a warning, it appears as an error.
edit: we saw with @amatissart that the error is also present in main, thus not introduced by this PR, and could be fixed by using the prop value instead of defaultValue in the component src/features/recommendation/CriteriaFilter.tsx
| <> | ||
| <Typography | ||
| variant="h6" | ||
| style={{ borderBottom: '1px solid #E7E5DB', marginBottom: '0.3em' }} |
There was a problem hiding this comment.
I would not recommend using inline style in our components.
The day we will add a Content Security Policy in the front end, inline style will be blocked if not explicity authorized. And authorizing inline style would be a security flaw.
There was a problem hiding this comment.
I updated the PR to use className instead of inline style.
Although your point about Content-Security-Policy is valid, enforcing such a policy everywhere would probably be constraining. Usage of inline styles is quite deeply baked into Material UI: see mui/material-ui#19938
There was a problem hiding this comment.
You're right.
I'm curious about how many mui components use inline style.
| <DateFilter setFilter={setFilter} /> | ||
| <LanguageFilter setFilter={setFilter} /> | ||
| <Collapse in={expanded} timeout="auto" unmountOnExit> | ||
| <Grid container spacing={4} style={{ marginBottom: '8px' }}> |
There was a problem hiding this comment.
Avoidable inline style.
| startIcon={!expanded ? <ExpandMore /> : <ExpandLess />} | ||
| aria-expanded={expanded} | ||
| aria-label="show more" | ||
| onClick={handleExpandClick} | ||
| style={{ |
There was a problem hiding this comment.
Avoidable inline style.
| startIcon={!expanded ? <ExpandMore /> : <ExpandLess />} | ||
| aria-expanded={expanded} | ||
| aria-label="show more" | ||
| onClick={handleExpandClick} | ||
| style={{ |
There was a problem hiding this comment.
Avoidable inline style.
| <DateFilter setFilter={setFilter} /> | ||
| <LanguageFilter setFilter={setFilter} /> | ||
| <Collapse in={expanded} timeout="auto" unmountOnExit> | ||
| <Grid container spacing={4} style={{ marginBottom: '8px' }}> |
There was a problem hiding this comment.
Avoidable inline style.
As discussed on Discord, updating the |
Description
This is a prerequisite to implement the new page "My rated videos", that will also include similar filters (on public/private ratings, etc.)
2 new components:
FilterSection: a generic filter section with an underlined titleChoicesFilterSection: a filter column, with multiple checkboxes defined by a list ofchoices.Used by
LanguageFilterandDateFilter.Screenshots