fix(filters): improving the add filter/divider UI. #31279
Conversation
| @@ -267,7 +267,6 @@ const TableElement = ({ table, ...props }: TableElementProps) => { | |||
| return ( | |||
| <ButtonGroup | |||
| css={css` | |||
| display: flex; | |||
There was a problem hiding this comment.
Moved this to be a default style for ButtonGroup, rather than an override.
| @@ -30,22 +31,28 @@ export default function ButtonGroup(props: ButtonGroupProps) { | |||
| role="group" | |||
| className={className} | |||
| css={{ | |||
| '& :nth-of-type(1):not(:nth-last-of-type(1))': { | |||
| display: 'flex', | |||
| '& > :nth-of-type(1):not(:nth-last-of-type(1))': { | |||
There was a problem hiding this comment.
Now that we have icons in buttons sometimes (which was always supported by AntD), I made these styles more specific, so they apply to the buttons themselves, and not elements within the buttons.
| borderTopLeftRadius: 0, | ||
| borderBottomLeftRadius: 0, | ||
| marginLeft: 0, | ||
| }, | ||
| ...(props.expand && { |
There was a problem hiding this comment.
Adding a new "expand" prop that makes buttons take up the available width (stretching).
Important side note that I GREATLY prefer adding props or classes to components, rather than adding overrides in their individual instances (that makes a mess).
| @@ -54,6 +54,7 @@ import { | |||
| LineChartOutlined, | |||
| LoadingOutlined, | |||
| MonitorOutlined, | |||
| PicCenterOutlined, | |||
There was a problem hiding this comment.
Adding a new icon that works well enough for filter bar dividers.
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTitlePane.tsx
Outdated
Show resolved
Hide resolved
…perset into add-filter-and-divider-buttons
|
Thanks @rusackas for taking this up! Buttons here look more like tabs to me and it's not as obvious that they add new elements. I would suggest going for either:
|
|
This is a good improvement already, I'm not against merging as "less bad". Maybe a design idea would be to move the "add divider" button under the last filter (?) Not sure if that makes sense |
|
Per @mistercrunch @justinpark and @michael-s-molina 's suggestions, I've moved the buttons to the bottom (like the Apply Filters button). Per @kasiazjc and @michael-s-molina 's suggestions, I've separated the buttons rather than using the split-button ButtonGroup component. Per @kasiazjc's request, I've gone with secondary rather than tertiary buttons. Definitely looks/feels better now. Someone please approve it, so I can go straighten some other crooked picture on a wall somewhere ;) |
|
|
Agreed... after everyone's feedback, things look much better than my first iteration. I set my standards too low, as I simply wanted the weird popover to die a fiery death. Once I'm done playing whack-a-mole with CI, I'll merge :D |
|
Just need a Cypress codeowner stamp from @geido @eschutho or @betodealmeida to squeeze this one through 🤞 |
SUMMARY
This has driven me crazy for too long! Finally fixing the weird pop-up to add filters or dividers, and just use sensible buttons!
I'll add some comments in the thread, hoping it helps reviewers.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION