#1144 Disable node coloring when uploading file with no expression #1153
Conversation
…sion spreadsheet
…ok.expression -> grnState.workbook.expression
web-client/public/js/update-app.js
Outdated
| const hasExpressionData = (sheets) => { | ||
| for (var property in sheets) { | ||
| if (property.match(ENDS_IN_EXPRESSION_REGEXP)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
This isn’t functionally incorrect, but along the lines of more current idioms when processing lists or objects, see if this can be written as a some call? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/some
Using some will make the code look something like this…
| const hasExpressionData = (sheets) => { | |
| for (var property in sheets) { | |
| if (property.match(ENDS_IN_EXPRESSION_REGEXP)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| }; | |
| const hasExpressionData = (sheets) => { | |
| return (/*expression with sheets*/).some( | |
| value => (/*expression with value */).match(ENDS_IN_EXPRESSION_REGEXP) | |
| ) | |
| }; |
…which to me sounds more concise, and objectively does not use side effects as well as easier for an optimizer to parallelize
I realize sheets isn’t an array, but there may be ways to take advantage of the simpler expressiveness of some vs. having a return true with a return false at the end (that’s why I have some placeholder /* */ comments in there instead of specific code; there may be other places too)
There was a problem hiding this comment.
I just modified it to use some call instead.
There was a problem hiding this comment.
You certainly modified it in graph.js, but does that mean that lines 515–522 above are duplicated?
web-client/public/js/graph.js
Outdated
| } | ||
| } | ||
| return false; | ||
| return Object.keys(sheets).some(property => property.match(ENDS_IN_EXPRESSION_REGEXP)); |
There was a problem hiding this comment.
Thanks, this looks good! But it looks like this function is duplicated in update-app.js? (see line 515 below) Should these functions be consolidated into one?
Sorry I didn’t notice this before…
There was a problem hiding this comment.
I didn't notice it either ....
web-client/public/js/update-app.js
Outdated
| const hasExpressionData = (sheets) => { | ||
| for (var property in sheets) { | ||
| if (property.match(ENDS_IN_EXPRESSION_REGEXP)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
You certainly modified it in graph.js, but does that mean that lines 515–522 above are duplicated?
No description provided.