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(explore): Scroll only table in Change Dataset and Edit Dataset Modals #12598
fix(explore): Scroll only table in Change Dataset and Edit Dataset Modals #12598
Conversation
…ticky_modal_elements
Codecov Report
@@ Coverage Diff @@
## master #12598 +/- ##
==========================================
- Coverage 66.79% 63.68% -3.12%
==========================================
Files 1015 486 -529
Lines 49676 29984 -19692
Branches 4847 0 -4847
==========================================
- Hits 33183 19095 -14088
+ Misses 16371 10889 -5482
+ Partials 122 0 -122
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Tested manually across safari,chrome,firefox 🟢 |
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.
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 with one small comment.
${({ scrollTable, theme }) => | ||
scrollTable && | ||
` | ||
height: 300px; |
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 we make this height configurable, or use something like 40vh
(40% of viewport height)?
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 a good idea. However, there is some calculation to do to make sure the height is just right for the modal to avoid showing its own scrollbar. I'll keep this in mind for an enhancement and provide a separate PR for the sake of getting this improvement in asap
SUMMARY
Closes #12488
Makes the table in the modal scrollable while keeping all the other elements sticky.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE Change Dataset Modal:
Change-dataset-BEFORE.mp4
AFTER Change Dataset Modal:
Change-dataset-AFTER.mp4
BEFORE Edit Dataset Modal:
Edit-dataset-BEFORE.mp4
AFTER Edit Dataset Modal:
Edit-dataset-AFTER.mp4
TEST PLAN
ADDITIONAL INFORMATION