-
Notifications
You must be signed in to change notification settings - Fork 0
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
Load local file #43
Load local file #43
Conversation
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.
Love the new functionality! Left a few nitpicky comments but overall everything works great. Attaching all the control containers to the controls object is a nice refactor too.
@@ -7,12 +7,12 @@ export function renderChart(cat) { | |||
cat.settings.sync(cat); | |||
//render the new chart with the current settings | |||
var dataFile = cat.controls.dataFileSelect.node().value; | |||
var dataFilePath = cat.config.dataURL + dataFile; | |||
var dataObject = cat.config.dataFiles.find(f => f.label == dataFile); |
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.
add this polyfill for find to support IE: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find#Polyfill
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.
Fixed in hotfixes
branch.
|
||
cat.controls.bootstrapButton = settingsSection | ||
cat.controls.bootstrapButton = cat.controls.environmentWrap | ||
.append("button") | ||
.text("Load Bootstrap") | ||
.on("click", function() { | ||
loadBootstrap(cat); |
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 calling loadRenderer()
but not clearing out the current chart. Does the chart need to be re-rendered?
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.
Maybe ... but initBoostrapConfig.js
goes away in favor of a more generalized approach in a downstream PR, so not going to worry about it ;)
.attr("type", "file") | ||
.attr("class", "file-load-input") | ||
.on("change", function() { | ||
if (this.value.slice(-4) == ".csv") { |
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.
Maybe toLowerCase()
this just to cover all the bases.
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.
Fixed in hotfixes
branch.
fr.onload = function(e) { | ||
//make sure the file is a csv | ||
|
||
//make an object for the file |
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.
Very cool.
|
||
//make an object for the file | ||
var dataObject = { | ||
label: files[0].name + " (loaded by user)", |
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.
Perhaps check the label against the current set of labels and add a (1) or something so the user can upload two files with the same name, like when they're editing the same data file to test edge cases.
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.
added timestamp to distinguish duplicates in hotfixes
branch
? { label: df, path: cat.config.dataURL, user_loaded: false } | ||
: df; | ||
}); | ||
console.log(cat.config.dataFiles); |
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.
stray clog
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.
Cleaned up in downstream PR
Thanks for the nice review @samussiah. Fixed the review comments in a new |
closes #40