Skip to content
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): make horizontal scroll appears in data panel #13566

Merged
merged 7 commits into from
Mar 24, 2021

Conversation

stephenLYZ
Copy link
Member

SUMMARY

Because the height of the tabpanel is higher than the height of the outer parent, the horizontal scroll bar is squeezed below.
Here need to minus some height to show horizontal scroll bar. Also the pagination's padding-bottom should be set to zero which causes a row is appearing bellow the component.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

image

After

Kapture

TEST PLAN

ADDITIONAL INFORMATION

@junlincc
Copy link
Member

junlincc commented Mar 11, 2021

Many thanks for the PR! 🙏

@villebro @rusackas
Hey Ville and Evan, Stephen is a developer that me and Yongjie "interviewed" last week, he picked out a task from bash! and opened this PR as requested. please review. :)

cc @ktmud as you recently touched this area as well.

@junlincc junlincc added new:contributor The author is a new contributor explore Namespace | Anything related to Explore labels Mar 11, 2021
@junlincc junlincc requested a review from rusackas March 11, 2021 21:54
@zhaoyongjie zhaoyongjie reopened this Mar 19, 2021
@junlincc
Copy link
Member

/testenv up

@apache apache deleted a comment from github-actions bot Mar 19, 2021
@apache apache deleted a comment from github-actions bot Mar 19, 2021
@junlincc
Copy link
Member

@kgabryje ^ you wanna take a look and let us know your thoughts? thanks!

@zhaoyongjie
Copy link
Member

@stephenLYZ could you fix the lint in this PR?

@github-actions
Copy link
Contributor

@junlincc Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment creation failed. Please check the Actions logs for details.

@stephenLYZ
Copy link
Member Author

@zhaoyongjie done

@@ -271,7 +271,7 @@ export const DataTablesPane = ({
height: 100%;

.ant-collapse-content {
height: 100%;
height: calc(100% - 33px);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use theme.gridUnit * 8 instead of 33px?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ye!It's better than hardcode

Copy link
Member Author

@stephenLYZ stephenLYZ Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, please review @kgabryje, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kgabryje
Copy link
Member

1 nit, other than that LGTM.
@junlincc What do you think about displaying horizontal scrollbar permanently so that user knows that there are more columns? It should work on Chrome and Safari (not Firefox though).

@stephenLYZ
Copy link
Member Author

1 nit, other than that LGTM.
@junlincc What do you think about displaying horizontal scrollbar permanently so that user knows that there are more columns? It should work on Chrome and Safari (not Firefox though).

How about show all columns like chart table which is responsive?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 21, 2021
@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #13566 (e39d79d) into master (5fca19d) will decrease coverage by 4.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13566      +/-   ##
==========================================
- Coverage   77.53%   73.11%   -4.43%     
==========================================
  Files         904      615     -289     
  Lines       45986    21876   -24110     
  Branches     5552     5812     +260     
==========================================
- Hits        35657    15995   -19662     
+ Misses      10195     5738    -4457     
- Partials      134      143       +9     
Flag Coverage Δ
cypress 56.44% <100.00%> (-0.77%) ⬇️
hive ?
javascript 63.13% <50.00%> (-0.25%) ⬇️
mysql ?
postgres ?
presto ?
python ?
sqlite ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...et-frontend/src/components/TableView/TableView.tsx 96.42% <ø> (ø)
...frontend/src/explore/components/DataTablesPane.tsx 62.96% <100.00%> (+0.93%) ⬆️
...s/nativeFilters/FilterBar/CascadeFilterControl.tsx 64.28% <0.00%> (-21.43%) ⬇️
...hboard/components/nativeFilters/FilterBar/state.ts 85.71% <0.00%> (-14.29%) ⬇️
...ponents/nativeFilters/FilterBar/CascadePopover.tsx 83.09% <0.00%> (-14.09%) ⬇️
...tend/src/filters/components/Select/controlPanel.ts 20.00% <0.00%> (-13.34%) ⬇️
...odal/FiltersConfigForm/FilterScope/FilterScope.tsx 86.20% <0.00%> (-9.95%) ⬇️
...src/dashboard/components/FiltersBadge/selectors.ts 80.58% <0.00%> (-9.90%) ⬇️
...ents/nativeFilters/FilterBar/FilterSets/Footer.tsx 47.61% <0.00%> (-8.64%) ⬇️
superset-frontend/src/common/components/Select.tsx 91.66% <0.00%> (-8.34%) ⬇️
... and 407 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 5fca19d...e39d79d. Read the comment docs.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link

@junlin-qa junlin-qa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responsiveness is probably better in this case. lets merge! 🎉 thank you stephen!!

@villebro villebro merged commit f18d14a into apache:master Mar 24, 2021
allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021
)

* fix(explore): make horizontal scroll appear

* fix(explore): 34px -> 33px

* fix(lint): dataTablesPane lint

* fix(lint): prettier dataTablesPane

* fix(explore): use gridUnit rather than hardcode

Co-authored-by: liuyaozong <[email protected]>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels explore Namespace | Anything related to Explore new:contributor The author is a new contributor size/M 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Horizontal scroll not working in explore data panel
7 participants