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

Improved Datatables View (and some other minor improvements as well) #3375

Closed
wants to merge 46 commits into from

Conversation

malcolmflint
Copy link

During my time at Yahoo/Oath as an Intern, I made improvements to the datatables implementation in the table visualization. Most notably I was able to improve the scrolling as well as add an export to csv button that will export the filtered table. I also improved the speed of rendering the visualization in part by removing the last metric column if there are no metrics given.

Malcolm Flint and others added 30 commits June 29, 2017 20:25
… and got the non metrics table sort of sorted
it
s been a while
Conflicts:
	setup.py
	superset/assets/package.json
	superset/assets/stylesheets/superset.css
	superset/assets/visualizations/filter_box.jsx
	superset/assets/visualizations/table.js
@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage remained the same at 69.287% when pulling b7971eb on malcolmflint:master into c944c61 on apache:master.

@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage remained the same at 69.287% when pulling 8d867cd on malcolmflint:master into c944c61 on apache:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.287% when pulling 8d867cd on malcolmflint:master into c944c61 on apache:master.

metadata.json Outdated
@@ -0,0 +1,12 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this file needed for?

Copy link
Author

Choose a reason for hiding this comment

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

My bad, that one snuck it's way in. I'll remove it. Just a sec

@@ -61,7 +61,6 @@
USER_MISSING_ERR = __("The user seems to have been deleted")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop all the changes to this file unless there's a good reason you need them

Copy link
Author

Choose a reason for hiding this comment

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

the reason to go to appbuilder.sm.auth_view.login (line 2231) is so that if someone were to use OAuth and define their own security manager it doesn't get stuck in a redirect loop. With default authentication, I believe it works exactly the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then that's fine

@@ -54,6 +54,11 @@
"datatables-bootstrap3-plugin": "^0.5.0",
"datatables.net": "^1.10.13",
"datatables.net-bs": "^1.10.12",
"datatables.net-buttons": "^1.3.1",
"datatables.net-buttons-bs": "^1.3.1",
"font-awesome": "^4.6.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we dropped this in favour of the fab shipped one.

Copy link
Author

Choose a reason for hiding this comment

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

dt buttons?

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment is for font-awesome

Copy link
Author

Choose a reason for hiding this comment

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

ah, I would be happy to remove the line provided it won't break anything. I'll do that now

showMetricNumber: PropTypes.bool,
origSelectedValues: PropTypes.object,
instantFiltering: PropTypes.bool,
sliceId: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we actually using sliceId for something?

Copy link
Author

Choose a reason for hiding this comment

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

It appears not. That was in there when I began work on the filter_box, so I added to prop_types for consistency...Let me know and I'll remove it from the entire file.

Copy link
Contributor

Choose a reason for hiding this comment

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

If sliceId is not used no need to keep it.

Copy link
Author

Choose a reason for hiding this comment

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

I will remove that now

@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage remained the same at 69.287% when pulling 92aa0a0 on malcolmflint:master into c944c61 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.287% when pulling 92aa0a0 on malcolmflint:master into c944c61 on apache:master.

2 similar comments
@coveralls
Copy link

coveralls commented Aug 25, 2017

Coverage Status

Coverage remained the same at 69.287% when pulling 92aa0a0 on malcolmflint:master into c944c61 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.287% when pulling 92aa0a0 on malcolmflint:master into c944c61 on apache:master.

@mistercrunch
Copy link
Member

Notice: this issue has been closed because it has been inactive for 240 days. Feel free to comment and request for this issue to be reopened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants