fix(chart-table): Scrollbar causing header + footer overflow #21064
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21064 +/- ##
==========================================
- Coverage 66.38% 66.29% -0.09%
==========================================
Files 1767 1769 +2
Lines 67232 67399 +167
Branches 7138 7175 +37
==========================================
+ Hits 44633 44685 +52
- Misses 20773 20880 +107
- Partials 1826 1834 +8
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 |
|
@geido Ephemeral environment spinning up at http://34.213.181.137:8080. Credentials are |
|
Hello @reesercollins it would be great if you could add a video or GIF before and after the changes. Thank you! |
| // eslint-disable-next-line react/no-array-index-key | ||
| <col | ||
| key={i} | ||
| width={x + (i === colWidths.length - 1 ? scrollBarSize : 0)} |
There was a problem hiding this comment.
@reesercollins just to be triple sure, do you have evidence that this isn't required any longer?
There was a problem hiding this comment.
I haven't been able to find any instances where this removal causes any breaking changes, however, I am not the most knowledgeable when it comes to the usage of this plugin within superset since our organization uses a custom table viz.
|
/testenv up |
|
@kgabryje Ephemeral environment spinning up at http://52.42.126.233:8080. Credentials are |
geido
requested review from
zhaoyongjie,
michael-s-molina,
AAfghahi,
lyndsiWilliams and
villebro
|
/testenv up |
|
@rusackas Container image not yet published for this PR. Please try again when build is complete. |
|
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details. |
There was a problem hiding this comment.
Approving this, as it seems fairly innocuous. I'd like to test it further on an ephemeral env, but that doesn't seem to be working, and I can't seem to find anyone with a PC to test this anyway.
Heads up to @jinghua-qa that we might want to keep an eye on table scrolling issues in future testing, and if we have the means, make sure that the related Issue in the PR description (using PC Chrome) is resolved once and for all.
To @geido's point, if this DOES cause any unforeseen issues, it should be easy to roll back! 🤞
|
Ephemeral environment shutdown and build artifacts deleted. |
mistercrunch
added
🏷️ bot
SUMMARY
This fixes a bug caused by bad (and unneeded) scrollbar compensating code. See the linked issue for more info.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
2022-08-15.11-40-10.mp4
After:
2022-08-15.11-41-09.mp4
TESTING INSTRUCTIONS
Try reproducing the bug in the linked issue. It should no longer appear.
ADDITIONAL INFORMATION