Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: Make the scrollbar appear inside the table #1310

Merged
merged 3 commits into from
Aug 24, 2021
Merged

fix: Make the scrollbar appear inside the table #1310

merged 3 commits into from
Aug 24, 2021

Conversation

geido
Copy link
Member

@geido geido commented Aug 13, 2021

SUMMARY

This PR only solves an issue with the scrollbar not appearing in the table container but at the far right. Read this comment for context

BEFORE

DEV.Pivot.Tabl.1.mp4

AFTER

DEV.Pivot.Tabl.mp4

TESTING INSTRUCTIONS

  1. Open Explore with a Pivot Table V2 viz
  2. Observe the table and scroll
  3. Make sure the scrollbar appears within the table container

ADDITIONAL INFORMATION

  • Has associated issue: [scroll]inconsistent scrolls with sticky header  apache/superset#15935
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@geido geido requested a review from a team as a code owner August 13, 2021 14:50
@vercel
Copy link

vercel bot commented Aug 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/EDQX4SQ3pbRcJUafnc7MpdMbBzMp
✅ Preview: https://superset-ui-git-fork-geido-fix-issue15935scroll-eb3a07-superset.vercel.app

@codecov
Copy link

codecov bot commented Aug 13, 2021

Codecov Report

Merging #1310 (5d6e14b) into master (1dfca35) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1310   +/-   ##
=======================================
  Coverage   29.94%   29.94%           
=======================================
  Files         485      487    +2     
  Lines        9825     9841   +16     
  Branches     1645     1647    +2     
=======================================
+ Hits         2942     2947    +5     
- Misses       6647     6658   +11     
  Partials      236      236           
Impacted Files Coverage Δ
...s/plugin-chart-pivot-table/src/PivotTableChart.tsx 0.00% <0.00%> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts 44.44% <0.00%> (-1.94%) ⬇️
...ugins/plugin-chart-echarts/src/Timeseries/types.ts 100.00% <0.00%> (ø)
...ugin-chart-echarts/src/Timeseries/controlPanel.tsx 40.00% <0.00%> (ø)
...chart-echarts/src/Timeseries/Area/controlPanel.tsx 50.00% <0.00%> (ø)
...chart-echarts/src/Timeseries/EchartsTimeseries.tsx 0.00% <0.00%> (ø)
...chart-echarts/src/Timeseries/Step/controlPanel.tsx 40.00% <0.00%> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <0.00%> (ø)
...rt-echarts/src/Timeseries/Regular/controlPanel.tsx 50.00% <0.00%> (ø)
...rts/src/MixedTimeseries/EchartsMixedTimeseries.tsx 0.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1dfca35...5d6e14b. Read the comment docs.

@rusackas
Copy link
Member

Code LGTM... just going to do a little npm link action to test it with tables of various widths/heights in different browsers.

@rusackas
Copy link
Member

rusackas commented Aug 13, 2021

When the table goes super wide, it scrolls far too much content. Only the table itself should scroll, not the header/toolbar above or the data preview below. I think you might just need to add a max-width to the new wrapper you've created.

scrollproblem2.mp4

@geido
Copy link
Member Author

geido commented Aug 16, 2021

Hey @rusackas I have included the fix for the X axis as well in this PR as you suggested. I have also updated the video in the PR description. Thanks!

@geido geido merged commit 1aad2d1 into apache-superset:master Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants