Remove Node Sequences Option #414

shreyasun · 2024-04-03T16:51:52Z

Fixes #349

…issues in tubemap rendering

…are there

adamnovak · 2024-04-09T15:18:10Z

I think the test is failing because &dataPath=default got added back to the expected link during the merge.

adamnovak

This looks like it should work, but there are some places where I would like the code cleaned up before merging it.

Also, for PRs that add new UI like this, it would be good to have a screenshot included to show how it looks.

src/components/HeaderForm.js

adamnovak · 2024-04-09T16:15:55Z

src/components/HeaderForm.js

@@ -825,6 +832,19 @@ class HeaderForm extends Component {
    this.setState({ simplify: !this.state.simplify });
  };

+  /* Function for toggling nodeSequences button, enabling client to make request for node sequences to be displayed or note */


I don't think nodeSequences is a variable or function name, and note is a typo. We could simplify the sentence to something like:

Suggested change

/* Function for toggling nodeSequences button, enabling client to make request for node sequences to be displayed or note */

/* Function for toggling display of node sequences */

adamnovak · 2024-04-09T16:19:08Z

src/components/HeaderForm.js

+  openPopup = () => {
+    this.setState({ popupOpen: !this.state.popupOpen });
+  };
+
+  closePopup = () => {
+    this.setState({ popupOpen: !this.state.popupOpen });
+  };


These are called openPopup and closePopup but really they are two copies of a toggle-the-popup function. Either they should always do what their names say, or there should just be one togglePopup.

adamnovak · 2024-04-09T16:28:10Z

package.json

@@ -49,6 +49,7 @@
    "react-scripts": "5.0.1",
    "react-select": "^5.7.3",
    "react-select-event": "^5.5.1",
+    "react-switch": "^7.0.0",


This is not actually the switch component documented at https://mui.com/material-ui/react-switch/ that I thought we were using. This is a completely different package documented at https://www.npmjs.com/package/react-switch#usage which has about the same API.

This one might be a little better, so maybe we should keep it?

adamnovak · 2024-04-09T16:29:35Z

src/end-to-end.test.js

@@ -339,7 +339,7 @@ describe("When we wait for it to load", () => {
 it("produces correct link for view before & after go is pressed", async () => {
  // First test that after pressing go, the link reflects the dat form
  const expectedLinkBRCA1 =
-    "http://localhost?name=snp1kg-BRCA1&tracks[0][trackFile]=exampleData%2Finternal%2Fsnp1kg-BRCA1.vg.xg&tracks[0][trackType]=graph&tracks[0][trackColorSettings][mainPalette]=greys&tracks[0][trackColorSettings][auxPalette]=ygreys&tracks[1][trackFile]=exampleData%2Finternal%2FNA12878-BRCA1.sorted.gam&tracks[1][trackType]=read&region=17%3A1-100&bedFile=exampleData%2Finternal%2Fsnp1kg-BRCA1.bed&dataType=built-in&simplify=false";
+    "http://localhost?name=snp1kg-BRCA1&tracks[0][trackFile]=exampleData%2Finternal%2Fsnp1kg-BRCA1.vg.xg&tracks[0][trackType]=graph&tracks[0][trackColorSettings][mainPalette]=greys&tracks[0][trackColorSettings][auxPalette]=ygreys&tracks[1][trackFile]=exampleData%2Finternal%2FNA12878-BRCA1.sorted.gam&tracks[1][trackType]=read&dataPath=default&region=17%3A1-100&bedFile=exampleData%2Finternal%2Fsnp1kg-BRCA1.bed&dataType=built-in&simplify=false&removeSequences=false";


You probably don't mean to add back dataPath.

Suggested change

"http://localhost?name=snp1kg-BRCA1&tracks[0][trackFile]=exampleData%2Finternal%2Fsnp1kg-BRCA1.vg.xg&tracks[0][trackType]=graph&tracks[0][trackColorSettings][mainPalette]=greys&tracks[0][trackColorSettings][auxPalette]=ygreys&tracks[1][trackFile]=exampleData%2Finternal%2FNA12878-BRCA1.sorted.gam&tracks[1][trackType]=read&dataPath=default&region=17%3A1-100&bedFile=exampleData%2Finternal%2Fsnp1kg-BRCA1.bed&dataType=built-in&simplify=false&removeSequences=false";

"http://localhost?name=snp1kg-BRCA1&tracks[0][trackFile]=exampleData%2Finternal%2Fsnp1kg-BRCA1.vg.xg&tracks[0][trackType]=graph&tracks[0][trackColorSettings][mainPalette]=greys&tracks[0][trackColorSettings][auxPalette]=ygreys&tracks[1][trackFile]=exampleData%2Finternal%2FNA12878-BRCA1.sorted.gam&tracks[1][trackType]=read&region=17%3A1-100&bedFile=exampleData%2Finternal%2Fsnp1kg-BRCA1.bed&dataType=built-in&simplify=false&removeSequences=false";

adamnovak · 2024-04-09T16:30:02Z

src/server.mjs

+    {
+      sequence: "AGCT"
+      id: "1"
+    ,


Suggested change

,

},

adamnovak · 2024-04-09T16:30:15Z

src/server.mjs

+  node: [
+    {
+      id: "1"
+    ,


Suggested change

,

},

adamnovak · 2024-04-09T16:30:32Z

src/server.mjs

+  ]
+  edge: []
+  path: []


Suggested change

]

edge: []

path: []

],

edge: [],

path: []

adamnovak · 2024-04-09T16:30:44Z

src/server.mjs

+  ]
+  edge: []
+  path: []


Suggested change

]

edge: []

path: []

],

edge: [],

path: []

adamnovak · 2024-04-09T16:31:42Z

src/server.mjs

+    return;
+  }
+  graph.node.forEach(function(node) {
+    console.log("node sequence to delete:", node)


We probably should cut this; people are going to remove sequence because there's a lot of it, and we don't want a lot of sequence going to the logs.

Suggested change

console.log("node sequence to delete:", node)

adamnovak

I don't think trying to use !this.openPopup as a close event handler is going to work. I think if you take the suggestions here the dialog will actually work when you go to open and close it manually; it looks like it is probably broken right now.

adamnovak · 2024-04-17T21:16:11Z

src/components/HeaderForm.js

+                      >
+                      <FontAwesomeIcon icon={faGear} /> Simplify
+                      </Button>
+                      <PopupDialog open={this.state.popupOpen} close={!this.openPopup} width="400px">


I think openPopup() maybe needs to be renamed to togglePopup()? And you would just pass it here? It looks like you are trying to negate the function and I don't think this will actually work if you run it and try and open the dialog (so maybe that warrants a test?).

Suggested change

<PopupDialog open={this.state.popupOpen} close={!this.openPopup} width="400px">

<PopupDialog open={this.state.popupOpen} close={this.togglePopup} width="400px">

adamnovak · 2024-04-17T21:18:11Z

src/components/HeaderForm.js

+    this.setState({ removeSequences: !this.state.removeSequences });
+  };
+
+  openPopup = () => {


This really toggles it.

Suggested change

openPopup = () => {

togglePopup = () => {

adamnovak · 2024-04-17T21:18:39Z

src/components/HeaderForm.js

+                  {!readsExist(this.state.tracks) && (
+                    <>
+                      <Button
+                        onClick={this.openPopup}


If we rename the function we'd change it here too.

Suggested change

onClick={this.openPopup}

onClick={this.togglePopup}

shreyasun and others added 12 commits March 12, 2024 09:02

added new function to remove node sequences from graph nodes

56473a5

Removed sequences from node object, however, running into some other …

d1b2a6b

…issues in tubemap rendering

The sequences are being removed and options are configured correctly

738fca5

Added a condition to prevent removing node sequences option if reads …

1f09ce7

…are there

Created a small popup containing both simplification buttons

951f1a9

Styling changes to ensure that buttons are laid out correctly

fd5e86e

Replaced buttons with toggle switches, and styled them

bc02c4b

fixed module error

1fcc03c

package json

2e76312

package-lock json

082540f

Fixed link

5a110ae

Merge branch 'master' into simplifyNodes

d866e61

adamnovak self-requested a review April 9, 2024 15:05

adamnovak requested changes Apr 9, 2024

View reviewed changes

shreyasun added 7 commits April 10, 2024 09:32

Resolved requested changes

a3760de

Adjusted comments

0373f45

Adjusted comments

5a8a8c2

removed a console.log

d328949

Changed link

ae52601

Changed link

1242308

Changed link

4273936

adamnovak requested changes Apr 17, 2024

View reviewed changes

resolved popup toggle

a387bb2

adamnovak approved these changes Apr 26, 2024

View reviewed changes

adamnovak merged commit 5ef0f26 into master Apr 26, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Node Sequences Option #414

Remove Node Sequences Option #414

shreyasun commented Apr 3, 2024

adamnovak commented Apr 9, 2024

adamnovak left a comment

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak Apr 9, 2024

adamnovak left a comment

adamnovak Apr 17, 2024

adamnovak Apr 17, 2024

adamnovak Apr 17, 2024

	/* Function for toggling nodeSequences button, enabling client to make request for node sequences to be displayed or note */
	/* Function for toggling display of node sequences */

	<PopupDialog open={this.state.popupOpen} close={!this.openPopup} width="400px">
	<PopupDialog open={this.state.popupOpen} close={this.togglePopup} width="400px">

Remove Node Sequences Option #414

Remove Node Sequences Option #414

Conversation

shreyasun commented Apr 3, 2024

adamnovak commented Apr 9, 2024

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamnovak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment