-
Notifications
You must be signed in to change notification settings - Fork 8
#1387 Correctly branching from beta #1607
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,14 +55,24 @@ export const filenameWithExtension = function (mode, genes, edges, type, extensi | |
| filename = filename.substr(0, filename.length - currentExtension[0].length); | ||
| } | ||
| if (mode === NETWORK_GRN_MODE && extension === "xlsx") { | ||
| source = $("input[name=expressionSource]:checked")[0].value; | ||
| if (source === "none") { | ||
| const anyExpressionChecked = $("input[name=workbookSheets]:checked") | ||
| .toArray() | ||
| .some( | ||
| el => el.value && (el.value.includes("expression") || el.value.includes("sigma")) | ||
| ); | ||
|
|
||
| if (anyExpressionChecked) { | ||
| source = $("input[name=expressionSource]:checked")[0].value; | ||
| if (source === "none") { | ||
| source = null; | ||
| } else if (source === "userInput") { | ||
| // only demos will have an expression source | ||
| source = grnState.workbook.expression.source | ||
| ? grnState.workbook.expression.source | ||
| : "user-data"; | ||
| } | ||
| } else { | ||
|
Comment on lines
+64
to
+74
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For now, we can keep this code as-is. But for future reference—and we’ll see if there’s time to discuss this at the meeting—what the code is doing here represents a higher-level programming pattern of imperative code whose sole result is the value of a single variable ( In broader practice, this latter alternative (which will also be good for Alex to know) is generally considered safer and more robust, so even though what you have here is functionally equivalent, in terms of programming craft, the alternative is recommended. We can discuss this at some future juncture; it doesn’t need to block this PR |
||
| source = null; | ||
| } else if (source === "userInput") { | ||
| // only demos will have an expression source | ||
| source = grnState.workbook.expression.source | ||
| ? grnState.workbook.expression.source | ||
| : "user-data"; | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -526,7 +536,10 @@ export const upload = function () { | |
| <label for='exportExcelExpressionSource-noneRadio' id='exportExcelExpressionSource-none' class='export-radio-label'>None</label> | ||
| </li> | ||
| `; | ||
| if (Object.keys(grnState.workbook.expression).length > 0) { | ||
| if ( | ||
| Object.keys(grnState.workbook.expression).length > 0 && | ||
| grnState.workbook.expression.hasOwnProperty("source") | ||
| ) { | ||
| const isChecked = grnState.nodeColoring.nodeColoringEnabled ? `checked="true"` : ""; | ||
| result += ` | ||
| <li> | ||
|
|
||
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.
The provenance of the branch looks good now, but it looks like your first commit (0793d5e) piggy-backed this file unintentionally. We can talk about how that might have happened this time and what can be done to avoid it, but in terms of finalizing this PR, can you redo the branch one more time and make sure that your committed changes have just your intended changes exclusively. The redo will also help you determine how/why this file found its way to your commit