-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use topojson input from node_modules/sane-topojson #5492
Conversation
devtools/test_dashboard/devtools.js
Outdated
@@ -5,6 +5,7 @@ | |||
var Fuse = require('fuse.js/dist/fuse.common.js'); | |||
var mocks = require('../../build/test_dashboard_mocks.json'); | |||
var credentials = require('../../build/credentials.json'); | |||
var constants = require('@src/plots/geo/constants'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't you want tasks/util/constants
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly the same as what is used in geo_test
var constants = require('@src/plots/geo/constants'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. We are not really using local files here and in geo_test
on this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
Fixed in 343faa8.
devtools/test_dashboard/devtools.js
Outdated
@@ -19,7 +19,7 @@ var Tabs = { | |||
Plotly.setPlotConfig({ | |||
|
|||
// use local topojson files | |||
topojsonURL: '../../vendor/topojson/', | |||
topojsonURL: '/node_modules/sane-topojson/dist/', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or it should be
topojsonURL: '../../node_modules/sane-topojson/dist/',
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 815d047.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 LGTM!
Removing
topojson
files fromvendor
folder to avoid duplicates on (dist
andvendor
) and simplify build process by using raw files fromsane-topojson/dist
iwithinnode_modules
folder.@plotly/plotly_js