#671 feat: add FilterList display mode for enum filters#681
Conversation
conradarcturus
left a comment
There was a problem hiding this comment.
This is a very solid first PR -- great job!
I have some stylistic comments about moving some of the style statements to CSS and 1 change about left-margins -- but that's all pretty small so I'll approve so you don't have to wait for a re-review.
The screenshot tests are failing but that's okay because you are intentionally updating the interface appearance. You can see the results if you tag update-baselines to automatically regenerate the screenshots.
| const [expanded, setExpanded] = useState(false); | ||
| const optionsRef = useClickOutside(() => { | ||
| setTimeout(() => setExpanded(false), 100); | ||
| // Only auto-collapse for dropdown modes; FilterList is a persistent filter UI |
There was a problem hiding this comment.
Thanks for the comment
| position: isFilterList ? undefined : 'relative', | ||
| display: 'flex', | ||
| width: isFilterList ? '100%' : undefined, | ||
| alignItems: isFilterList ? 'flex-start' : 'center', | ||
| flexDirection: isFilterList ? 'column' : 'row', |
There was a problem hiding this comment.
Oof there is a lot of style exceptions. As you saw when you worked on this component, I generally avoided css and instead inlined style. That helps some comprehension & control over the styles. However I have since learned that was a Facebook company thing.
When its compact it isn't a big deal, but here it is taking up a lot of space. Can you instead give this a css class name (eg but not necessary SelectorButtons) and move all of these styles into controls.css for .SelectorButtons and .filterList .SelectorButtons
| display: 'flex', | ||
| flexDirection: 'column', | ||
| width: '100%', | ||
| alignItems: 'flex-start', | ||
| alignSelf: 'stretch', |
There was a problem hiding this comment.
Can you put these in controls.css under ".selector.filterList`?
| return ( | ||
| <div | ||
| ref={containerRef} | ||
| style={{ |
There was a problem hiding this comment.
For this (and the ButtonList display option above) -- we should probably move these styles to the CSS and give this component a classname eg. SelectorOptions. I will note though that ButtonList's marginLeft should stay like it is. Also, FilterList should also use that marginLeft trick to indent the buttons.
| color: 'inherit', | ||
| cursor: 'pointer', | ||
| fontSize: '0.9em', | ||
| padding: '0.2em 3.5em', |
There was a problem hiding this comment.
3.5em horizontal padding is very manual -- but I am not certain about the alternative (fill the whole row) so I'll differ to your judgment.
| case SelectorDisplay.FilterList: | ||
| style.border = 'none'; | ||
| style.borderRadius = '0.5em'; | ||
| style.padding = '0.2em 0.4em'; |
There was a problem hiding this comment.
minor nit: I usually use multiples of 0.25em (0.5, 0.75, 0.125...) to keep a consistent padding pattern, but I don't think people would notice anyway.
| style.borderRadius = '1em'; | ||
| if (!isSelected) style.border = '0.125em solid var(--color-button-secondary)'; | ||
| break; | ||
| case SelectorDisplay.FilterList: |
There was a problem hiding this comment.
This is good. It's annoying we have so many manual styles here but when I used to have this all in CSS there were so many conflicting statements, it was easier to do it like this.
Thank you Conrad, I have moved over to the master branch with a new PR containing the requested changes! |
Fixes #671
Summary: Replaced dropdown/button displays for enum filters with a new Filter List display mode that shows options as a compact vertical list, showing the first 4 with an "Expand All" toggle when there are 5 or more.
Out of scope/Future work: The dual meaning of expanded state (dropdown open vs filter list expanded) could be split into two separate state variables for clarity.
Test Plan and Screenshots
How to test the changes in this PR: Run npm run dev, open the Filter panel, then verify enum filters show as vertical lists and "Expand All" works.
Checklist
Feel free to check off or just delete items in this section as you have completed them.
Summary
Testing
npm run lintnpm run buildnpm run testnpm run dev-- tried out the website directlyChanges
Visual changes
Data changes
public/data/public/data/src/features/data/including how we aggregate data or compute derived valuesInternal changes
Docs