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

[explorev2] improving the scrolling/scrollbars placement #1840

Merged
merged 1 commit into from
Dec 16, 2016

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Dec 14, 2016

For the controls, moving the scrollbar outside the panel as opposed to inside the panel.
Before:
screen shot 2016-12-14 at 3 08 57 pm
After:
screen shot 2016-12-14 at 3 18 03 pm

Also, in v1, the chart container doesn't allow to overflow-x, unless the viz's CSS override that. Matching this behavior here so that the table view doesn't show an X axis scrollbar.

@@ -186,7 +186,6 @@ class ChartContainer extends React.Component {
id={this.props.containerId}
ref={(ref) => { this.chartContainerRef = ref; }}
className={this.props.viz_type}
style={{ overflowX: 'auto' }}
Copy link

Choose a reason for hiding this comment

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

this was added because there are some charts that don't scale to the width of the container, and so a scrollbar was needed to see the rest of the chart. i thought i did see this functionality in v1. i will try and find an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a bit of code archeology for this as I remembered fixing the call. overflow-x was introduced here 2d0ebea, but wasn't valid React style, so I fixed the syntax here c155857 , but by fixing it I really just brought something we didn't want.

Here are example of viz that override to overflow-x to set it to auto:
https://github.com/airbnb/superset/blob/master/superset/assets/visualizations/nvd3_vis.css#L15
https://github.com/airbnb/superset/blob/master/superset/assets/visualizations/nvd3_vis.css#L28

Copy link
Member Author

@mistercrunch mistercrunch Dec 15, 2016

Choose a reason for hiding this comment

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

Somehow setting overflow-x to auto makes the table and pivot table viz show too many scrollbars because of the way the "fixed header" scrolling is implemented

Copy link

Choose a reason for hiding this comment

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

i'm ok with taking this out. i'll add an issue to investigate how viz's are scaled width wise, to make sure there aren't cases where part of the viz is cut off.

@mistercrunch mistercrunch merged commit 007ee88 into apache:master Dec 16, 2016
@mistercrunch mistercrunch deleted the scroll branch December 16, 2016 16:06
@dpgaspar dpgaspar mentioned this pull request Jun 23, 2022
9 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.15.1 labels Feb 20, 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 🚢 0.15.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants