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.

Sign up for GitHub

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

Jump to bottom

Refactor around how visualizations/*.js are required #913

Merged
merged 4 commits into from
Aug 12, 2016
Merged

Refactor around how visualizations/*.js are required #913

merged 4 commits into from
Aug 12, 2016

Conversation

mistercrunch
Copy link
Member

@mistercrunch mistercrunch commented Aug 10, 2016 •
edited
Loading

  • Moved all require calls of visualizations to not be done "on the fly", this allow for commenting out the ones you don't need while iterating on one, reducing webpack builds from ~12s to ~4s
  • Broke down caravel.js in to dates.js and colors.js to avoid circular deps
  • Refactored filter_box.js to use React / jsx and linted it

@georgeke @ascott @bkyryliuk

@coveralls
Copy link

coveralls commented Aug 10, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.435% when pulling cf702472f67b2a0c2d9b0bfd564b2aeca41ec52c on mistercrunch:refactor into d6bb8c6 on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 10, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.435% when pulling cf702472f67b2a0c2d9b0bfd564b2aeca41ec52c on mistercrunch:refactor into d6bb8c6 on airbnb:master.

@xrmx
Copy link
Contributor

xrmx commented Aug 10, 2016

@mistercrunch this is going to clash with #888, the one that gets in later needs to be updated

ascott
ascott reviewed Aug 10, 2016
View reviewed changes
caravel/assets/javascripts/modules/caravel.js
let slice;
function getParam(name) {
name = name.replace(/[\[]/, '\\[').replace(/[\]]/, '\\]');
const regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
const newName = name.replace(/[\[]/, '\\[').replace(/[\]]/, '\\]');
Copy link
Contributor

Choose a reason for hiding this comment

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

could call this formattedName to be more specific.

@mistercrunch
Copy link
Member Author

@xrmx , I just merged #888 and will deal with merge conflicts on my end

@coveralls
Copy link

coveralls commented Aug 11, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.002% when pulling 2ed5b39 on mistercrunch:refactor into bcbe08b on airbnb:master.

@mistercrunch mistercrunch force-pushed the refactor branch 2 times, most recently from 3ee8d7b to 4d5579e Compare August 11, 2016 06:05
@coveralls
Copy link

coveralls commented Aug 11, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 80.83% when pulling 4d5579ecd0be606f0bc5fd01bc57fceb894fe73b on mistercrunch:refactor into 9b3b1f6 on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 11, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 81.237% when pulling 4d5579ecd0be606f0bc5fd01bc57fceb894fe73b on mistercrunch:refactor into 9b3b1f6 on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 11, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 81.237% when pulling 321b263d018b11e1f3caa578ea0809654301f2b6 on mistercrunch:refactor into 9b3b1f6 on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.237% when pulling a5bacb9342d652afd78e539f3d3936a8a339b7ef on mistercrunch:refactor into 198226a on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.237% when pulling bf77895017d57132f14f61c8e9b1c51db2716067 on mistercrunch:refactor into 198226a on airbnb:master.

@coveralls
Copy link

coveralls commented Aug 12, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 81.237% when pulling f005a27 on mistercrunch:refactor into 198226a on airbnb:master.

@mistercrunch mistercrunch merged commit e243a14 into apache:master Aug 12, 2016
@mistercrunch mistercrunch deleted the refactor branch August 12, 2016 04:39
zhaoyongjie added a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
@zhaoyongjie
chore: move all controls for NVD3 and Big Number charts to separate r…
8aa54f9 
…ows (apache#913)
zhaoyongjie added a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
@zhaoyongjie
chore: move all controls for NVD3 and Big Number charts to separate r…
6a93664 
…ows (apache#913)
zhaoyongjie added a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
@zhaoyongjie
chore: move all controls for NVD3 and Big Number charts to separate r…
5913982 
…ows (apache#913)
zhaoyongjie added a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
@zhaoyongjie
chore: move all controls for NVD3 and Big Number charts to separate r…
114d78e 
…ows (apache#913)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.11.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mistercrunch @coveralls @xrmx @ascott