-
Notifications
You must be signed in to change notification settings - Fork 13.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(dashboard): Show the filters popover behind the dashboard header when scrolling #15933
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15933 +/- ##
=======================================
Coverage 76.92% 76.92%
=======================================
Files 988 988
Lines 52302 52307 +5
Branches 7123 7126 +3
=======================================
+ Hits 40232 40238 +6
+ Misses 11845 11844 -1
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@rusackas Ephemeral environment spinning up at http://34.218.47.29:8080. Credentials are |
@@ -283,6 +285,13 @@ const DashboardBuilder: FC<DashboardBuilderProps> = () => { | |||
</DragDroppable> | |||
</StyledHeader> | |||
<StyledContent fullSizeChartId={fullSizeChartId}> | |||
<Global | |||
styles={css` |
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.
I wonder if styles
is any more performant than css
or if we should just make this css
for future consistency/themability. ¯\_(ツ)_/¯
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.
One little nit, but not a blocker.
Really appreciate the inline comments for the rationale about the z-index values, btw. Seeing more need every day to add a z-index registry (a bunch of defined "layers") to the Theme.
I will appreciate an eye from @michael-s-molina as well on this as he worked on similar issues in the past |
/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true |
@michael-s-molina Ephemeral environment spinning up at http://34.219.220.235:8080. Credentials are |
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. I did some tests with the native filters enabled and it's working as intended.
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 on UI
Ephemeral environment shutdown and build artifacts deleted. |
…when scrolling (apache#15933) * Fix z-index * Fix z-index dropdown and maximized chart filters
…when scrolling (apache#15933) * Fix z-index * Fix z-index dropdown and maximized chart filters
…when scrolling (apache#15933) * Fix z-index * Fix z-index dropdown and maximized chart filters
…when scrolling (apache#15933) * Fix z-index * Fix z-index dropdown and maximized chart filters
SUMMARY
This PR fixes the z-index of the dashboard header to make sure that the Filters Popover does not show over the Dashboard header.
Fixes: #15873
BEFORE
AFTER
FCC.New.Coder.Su.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION