-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
fix: Styling fixes for horizontal filter bar #22337
Conversation
@kasiazjc @kgabryje What do you think about inverting the positions of the empty message and the Add Filters? That way, the gear icon, and Add Filters will keep a constant position when interacting with the bar. So with this, we would have:
WDYT? |
@michael-s-molina Thank you for suggestions, applied both changes. |
Codecov Report
@@ Coverage Diff @@
## master #22337 +/- ##
==========================================
- Coverage 66.85% 66.85% -0.01%
==========================================
Files 1847 1847
Lines 70561 70560 -1
Branches 7737 7737
==========================================
- Hits 47174 47173 -1
Misses 21380 21380
Partials 2007 2007
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@michael-s-molina Spacings between the COMPONENTS are the same - 16px. The problem is that the icon component has some empty space around it. When we start using the icons from the new design system, the problem will be solved, since those new icons don't have spacing around them. CC @kasiazjc |
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.
LGTM. Thank you for the fixes and for addressing the comments @kgabryje.
`; | ||
|
||
const Wrapper = styled.div` | ||
${({ theme }) => ` | ||
padding: ${theme.gridUnit}px ${theme.gridUnit * 2}px; | ||
padding: ${theme.gridUnit * 3}px ${theme.gridUnit * 2}px ${ | ||
theme.gridUnit |
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.
that's some goofy formatting, but if the linter's happy, I'm happy.
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.
Yeah I don't like it either but that's what prettier came up with :(
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.
Code LGTM! Will try to remember to spin up a test env when CI is ready for that.
(cherry picked from commit d2b76a8)
SUMMARY
A few styling fixes related to horizontal native filters bar (and 1 for vertical):
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CC @kasiazjc @codyml for review