-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
refactor(dashboard): [chart-maximize-mode]put chart full-size state in redux #15384
Conversation
could you fix test? |
Codecov Report
@@ Coverage Diff @@
## master #15384 +/- ##
===========================================
+ Coverage 58.93% 77.09% +18.16%
===========================================
Files 373 975 +602
Lines 12014 50663 +38649
Branches 2945 6224 +3279
===========================================
+ Hits 7080 39059 +31979
- Misses 4755 11393 +6638
- Partials 179 211 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
grid-column: 2; | ||
grid-row: 2; | ||
z-index: 1; | ||
z-index: ${({ fullSizeChartId }) => (fullSizeChartId ? 1000 : 1)}; |
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 think z-index: 2
also enough, I think better not to use big zIndexes because if need later override them it will be more difficult
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'd like to make a place to store z-index values as constants so that they are all collected in one place and can be adjusted and compared easily. Maybe this PR isn't the place to do that though.
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 agreed. There seems to be no place to manage all z-index values.
Other things look ok, checked on local env, after this fix can be merged |
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.
Thanks for the fix!
19397dd
to
9d8e873
Compare
…n redux (apache#15384) * refactor(dashboard): [chart-maximize-mode]put full-size status to redux * fix: ci
…n redux (apache#15384) * refactor(dashboard): [chart-maximize-mode]put full-size status to redux * fix: ci
…n redux (apache#15384) * refactor(dashboard): [chart-maximize-mode]put full-size status to redux * fix: ci
SUMMARY
Currently, the zIndex of dashboard's content is
1
, which is lower than dashboard's title panel(2
). We should set that to higher in chart maximize mode. In order to achieve this requirement, I put the full-size state in redux so that the parent element can get this.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
before
2021-06-25.4.08.04.mov
after
2021-06-25.4.09.07.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION