-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: Make filters and dividers display horizontally in horizontal native filters filter bar #22169
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22169 +/- ##
==========================================
- Coverage 66.92% 66.91% -0.01%
==========================================
Files 1835 1840 +5
Lines 69988 70081 +93
Branches 7612 7636 +24
==========================================
+ Hits 46839 46896 +57
- Misses 21183 21218 +35
- Partials 1966 1967 +1
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 |
/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true |
@kgabryje Ephemeral environment spinning up at http://35.91.79.71:8080. Credentials are |
/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true |
@kgabryje Ephemeral environment spinning up at http://54.244.77.71:8080. Credentials are |
12fb8a4
to
9567b5e
Compare
/testenv up FEATURE_HORIZONTAL_FILTER_BAR=true |
@kgabryje Ephemeral environment spinning up at http://35.88.186.83:8080. Credentials are |
<FilterControlContainer | ||
layout={ | ||
orientation === FilterBarOrientation.HORIZONTAL && !overflow | ||
? 'horizontal' |
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.
nit: should we use an enum for horizontal
and vertical
layout? Maybe we could even simply use FilterBarOrientation
for that?
margin-bottom: ${theme.gridUnit * 4}px; | ||
`} | ||
> | ||
<Tooltip overlay={titleIsTruncated ? <strong>{title}</strong> : null}> |
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.
Should the tooltip content be bold? Can't see that requirement in the designs
@@ -270,7 +277,7 @@ const FilterValue: React.FC<FilterProps> = ({ | |||
height={HEIGHT} | |||
width="100%" | |||
showOverflow={showOverflow} | |||
formData={formData} | |||
formData={formDataWithDisplayParams} |
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.
hmmm I'm not sure if orientation and overflow belongs to formData
... I'd rather add a new optional prop to SuperChart
like displaySetting: Record<string, any>
and put those values there.
A few more comments:
Screen.Recording.2022-11-24.at.18.36.15.mov |
However, those are all minor problems and this PR looks great! I'm happy to merge it now and address the comments in separate PRs |
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.
The comments are non blocking since this is locked behind a feature flag. LGTM
Ephemeral environment shutdown and build artifacts deleted. |
SUMMARY
This PR adds filters and dividers to the horizontal filter bar, based off of @kgabryje's #22042 with some additional styling work on top. Dividers and filter wrappers (labels + spacing) should look correct, though the filter input controls themselves are not yet styled in this PR. Known issues that will be covered in future PRs:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
HORIZONTAL_FILTER_BAR