Decoupling tabs in the experiment page and display them after validating #375

upendrakumbham · 2023-12-12T16:41:37Z

Decoupling tabs in the experiment page and display them after validating

Requirements:

- Result, supplementary and download

tabs validate based on required props and common props
Display a tab if all the validations are passed
Don't display a tab if the validations failed and give a message to the user about why the tab is not visible to the user

Cell-plots part of the Results tab

Validate as above tabs, and display or hide based on validation status.
E-ANND- (AnnData)- inferred cell type ontology and authors labels should available, cell-plots should display otherwise display an error message to the user and hide tab. Unsupervised clusters are optional for the validation.

MarkerGenes and Anatomogram tabs

If marker genes are unavailable for E-ANND- (AnnData) experiments, don’t show the marker genes tab to the user.
If anatomograms are not available for the experiments, hide the tab from the user. Don't give any information to the user, about why we are hiding the tab.
Cell-plots tab for the (AnnData) experiments , ks clusters not available, we should enable tab without these ks (skip validation for the ks for Anndata experiments)

Error message: Error-Something went wrong. There is no data to display. Please contact the team - ](https://www.ebi.ac.uk/about/contact/support/gxasc).

…validate-before-displaying

lingyun1010 · 2024-01-09T14:35:18Z

The conditional operator hierachy looks good and we need to apply more props check for each tab I think.

upendrakumbham · 2024-01-09T15:16:32Z

The conditional operator hierachy looks good and we need to apply more props check for each tab I think.

Sure, thanks for the review and comment.

ke4 · 2024-01-18T17:36:06Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js


 const TopRibbon = ({tabs, routeProps}) =>
  <ul className={`tabs`}>
    {
      tabs.map((tab) =>
        <li title={tab.name} key={tab.type} className={`tabs-title`}>
+          {
+            tab.type === 'results' && Array.isArray(tab.props.ks) ? console.log("result1 **************", tabTypeComponent.push({'results' : TSnePlotViewRoute})) :


@upendrakumbham Could you clean this code please? We are watching Uncle Bob's Clean Code videos to apply his teaching in our every day coding.
I would like you to find yourself why this code is not clean and I would like you to change it. It is not that hard.
If you struggle with it, then please ping me and I can help, but I would really like to see that you can do it yourself.
Thanks

as per @lingyun1010 review comment, I have created JS functions to validate tabs and it's props.

Please review and provide comments

ke4 · 2024-01-31T14:46:22Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

-  'downloads' : DownloadsRoute
+let tabTypeComponent = []
+
+function validateCommonRequiredProps({speciesName},{atlasUrl},{experimentAccession}) {


I am a bit confused why we need this function at all.
We defined the types of the props in the definition of the prop types and we also defined whether they are required or not.
Why do we need another way to validate all of these again?

upendrakumbham · 2024-02-13T10:34:57Z

@karoy, please review this PR. My local setup has throwing 8000 port already bind issue when I try to test this PR. I will fix it and test, meanwhile suggest issue local experiment.

ke4

I added 2 comments that I would like to consider.
Thanks for your work on this

ke4 · 2024-02-15T09:30:41Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+
+function enableExperimentPageTab(tab) {
+  if (tab.type === 'results') {
+    if (Array.isArray(tab.props.ks) && (tab.props.plotTypesAndOptions)) {


This can be cleaned a bit more:

it is repetitive

part of the validation can be extracted to a well named function to make it clean what it is doing and hide the details

ke4 · 2024-02-15T09:33:30Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+      tabTypeComponent.push({'supplementary-information': SupplementaryInformationRoute})
+      return tab.name;
+    }
+  } else {


What happens if we are going to have a new tab.type?
For example this experiment also has a Plots tab.
With this current code it is going to be a download tab.
Could you please think it over if that's exactly we want here or something else?
Thanks

I got what you mean, I will work on this and let you know.

…lidations

lingyun1010 · 2024-02-20T10:39:18Z

Please check the variable tabTypeComponent push logic, if you check/console log tabTypeComponent after each time you push a new item. You may find duplicate items as shown in my screenshot below. Secondly, when you try to click each tab the size of tabTypeComponent will increase too.

lingyun1010

Please check my comments regarding the array tabTypeComponent

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

ke4

I added a longish comment.
Could you please consider them?
Ping me if you don't understand me.
Thanks

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

Signed-off-by: upendrakumbham <[email protected]>

…validate-before-displaying

Signed-off-by: upendrakumbham <[email protected]>

…nload' tabs Signed-off-by: upendrakumbham <[email protected]>

Signed-off-by: upendrakumbham <[email protected]>

…n any one of the inner tabs visibility Signed-off-by: upendrakumbham <[email protected]>

Signed-off-by: upendrakumbham <[email protected]>

…tab validation of Annadata experiment Signed-off-by: upendrakumbham <[email protected]>

lingyun1010

A general review comment first: please remove any developing test codes, such as console.log, and remove ; after command to keep the same coding style.

ke4

@upendrakumbham I started to review this PR, but I think either I don't really understand the intention of your code or it has some misunderstanding from your side. I have not finished the review as we need to clear the above mentioned misunderstanding.
Overall: the code is not very clean. It needs a lot of refactoring.
I would like to have a call to discuss how to go further with this PR.
Thanks

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

ke4 · 2024-09-10T09:25:55Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

-    search: PropTypes.string.isRequired,
-    hash: PropTypes.string.isRequired
-  }).isRequired
+    pathname: PropTypes.string.isRequired,


I don't know what you meant by This one is fixed. Sadly nothing has changed here.

Signed-off-by: upendrakumbham <[email protected]>

lingyun1010 · 2024-09-10T11:00:43Z

I had a deeper review and I think it's better to leave some commets asap.

I tested the Results tab props, including ks, ksWithMarkerGenes, metadata, plotTypesAndOptions, defaultPlotMethodAndParameterisation and anatomogram, by setting them as empty individually. As far as what I can see, the props plotTypesAndOptions and defaultPlotMethodAndParameterisation are not working properly. What we expect is if they are empty, we still show the anatomogram tab in Results tab, and the other horizontal tabs too. Please double check it and fix the senario.
In the console, some logs are found but I don't think it's necessary for debugging or reporting, please take a look.

I made some changes on TSnePlotViewRoute.js for inferred cell type marker gene features, please pull the latest develop branch changes and install the latest version packages and also pay attention to the conflicts please.

lingyun1010

Some code formats and data type improvements, apart from the functionality fix I commented before. Besides, we don't have to push any changes to package-lock.json, or we don't need to have them in the repo at all actually, so please also remove any commits on pakcage-lock.json to keep your PR more clean and slim.

lingyun1010 · 2024-09-10T11:06:07Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+                            return false
+                        }
+                    }
+                });


; nearby here, please remove.

Sorry, I didn't commit TsnePlotViewRouter before your review. Now I fixed all format issues as you mentioned here for both files

I search for ; , so no more

lingyun1010 · 2024-09-10T11:08:33Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+
+    const [enableResultTab, setEnableResultTab] = useState(false)
+
+


Extra empty lines. Please check your codes and make it as clean as possible, I also noticed, you've changed some original codes indention. Please double check the commands you've modified, maybe it's easier to be observed in github.

lingyun1010 · 2024-09-10T11:09:24Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

@@ -48,10 +49,20 @@ class TSnePlotViewRoute extends React.Component {
  }

  render() {
+
+    console.log(this.props)


Please deleted testing logs while developing.

lingyun1010 · 2024-09-10T11:09:55Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

    const { location, match, history } = this.props
    const { atlasUrl, suggesterEndpoint, defaultPlotMethodAndParameterisation } = this.props
    const { species, experimentAccession, accessKey, ks, ksWithMarkerGenes, plotTypesAndOptions, metadata, anatomogram } = this.props
    const search = URI(location.search).search(true)
+    if(species === `homo sapiens` && Object.keys(anatomogram).length >0){


Please add a space after > for better coding style.

Also we would use spece before ( and after ) with if condition. Please fix the same code format like this.

Many ; has not been removed in this file at least, please double check for all files.

lingyun1010 · 2024-09-10T11:15:19Z

app/src/main/javascript/bundles/experiment-page/src/TabConfig.js

+
+export const isEmptyArray = (value) => isObjectEmpty(value) && Array.isArray(value);
+
+export const tabValidations = new Map([


You can use object data type for tabValidations, tabCommonValidations and innerTabValidations, which is more common and easier for implementing and understanding.

lingyun1010 · 2024-09-10T11:16:41Z

docker/docker-compose-build-all.yml

@@ -15,8 +15,6 @@ services:
    extends:
      service: gradle
      file: docker-compose-gradle.yml
-    ports:


Is this a mistake? Please revert it back. And Line 32, 33.

We don't need ports snippet in this build-all yml file , ok I can revert to remove confusion.

If we have this, we getting error while running SCXA containers in my Laptop at least.

Signed-off-by: upendrakumbham <[email protected]>

upendrakumbham · 2024-09-10T12:00:44Z

I had a deeper review and I think it's better to leave some commets asap.

I tested the Results tab props, including ks, ksWithMarkerGenes, metadata, plotTypesAndOptions, defaultPlotMethodAndParameterisation and anatomogram, by setting them as empty individually. As far as what I can see, the props plotTypesAndOptions and defaultPlotMethodAndParameterisation are not working properly. What we expect is if they are empty, we still show the anatomogram tab in Results tab, and the other horizontal tabs too. Please double check it and fix the senario.

In the console, some logs are found but I don't think it's necessary for debugging or reporting, please take a look.

3. I made some changes on `TSnePlotViewRoute.js` for inferred cell type marker gene features, please pull the latest develop branch changes and install the latest version packages and also pay attention to the conflicts please.

For point 1: the props defaultPlotMethodAndParameterisation are not working properly. I have some doubts about showing some existing code issues. Can we have a call around 1:30 pm if you are free to discuss.

upendrakumbham · 2024-09-10T12:38:48Z

I had a deeper review and I think it's better to leave some commets asap.

I tested the Results tab props, including ks, ksWithMarkerGenes, metadata, plotTypesAndOptions, defaultPlotMethodAndParameterisation and anatomogram, by setting them as empty individually. As far as what I can see, the props plotTypesAndOptions and defaultPlotMethodAndParameterisation are not working properly. What we expect is if they are empty, we still show the anatomogram tab in Results tab, and the other horizontal tabs too. Please double check it and fix the senario.

In the console, some logs are found but I don't think it's necessary for debugging or reporting, please take a look.

3. I made some changes on `TSnePlotViewRoute.js` for inferred cell type marker gene features, please pull the latest develop branch changes and install the latest version packages and also pay attention to the conflicts please.

For point 1: the props defaultPlotMethodAndParameterisation are not working properly. I have some doubts about showing some existing code issues. Can we have a call around 1:30 pm if you are free to discuss.

plotTypesAndOptions is not a required field as per our validations doc, so I didn't add it as a validation field. Do you want me to include that as well?

ke4

I added a lot of change requests.
It is probably the 1st round. If the code is getting cleaner, we might need another refactor.
Thanks

ke4 · 2024-09-10T11:20:29Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

-  match: PropTypes.object.isRequired,
-  location: PropTypes.object.isRequired,
-  history: PropTypes.object.isRequired
+    match: PropTypes.object.isRequired,


Could you group together the code parts that belongs to PropTypes?
Thanks

ke4 · 2024-09-10T11:27:01Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+            const propValue = commonProps.valueOf(commonProp);
+            if (propValue === 'undefined' || propValue == '' || propValue == null) {
+                console.log(`${tab.type} data missing the required value for the attribute ${commonProp}`);
+                shouldRender = false;


This is unnecessary, in the next line we return with false and the local variable shouldRender won't be used.

ke4 · 2024-09-10T11:31:11Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+            if (propValue === 'undefined' || propValue == '' || propValue == null) {
+                console.log(`${tab.type} data missing the required value for the attribute ${commonProp}`);
+                shouldRender = false;
+                return false;


This is going to return false and this tab won't be rendered, but there is no message for the user why that tab is not rendered.
Is that what we want?
If yes, then this validation is already happening with the PropTypes required parameter, AFAIK.
I am a bit confused why we need this.

ke4 · 2024-09-10T11:41:43Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+                    let tableHeader = [];
+                    let TableData = [];
+
+                    if (isEmptyArray(table)) {


As you just defined table as an empty array: [] this should be always true.
Please remove this line.
Thanks

ke4 · 2024-09-10T13:07:11Z

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js

+                            return false; // Early return on failure
+                        }
+                    }
+                    if (isEmptyArray(tableHeader)) {


The same as with the table var.
As you just defined tableHeader as an empty array: [] this should be always true.
Please remove this line.
Thanks

ke4 · 2024-09-10T15:23:19Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

+        if (requiredProp === 'defaultPlotMethodAndParameterisation') {
+          console.log("defaultPlotMethodAndParameterisation"+JSON.stringify(defaultPlotMethodAndParameterisation))
+          if (defaultPlotMethodAndParameterisation.length == 0) {
+            console.log(`${route.title}: Missing selectedPlotOption and selectedPlotType data`);


Could you remove this line, please?

ke4 · 2024-09-10T15:23:26Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

+
+        if (requiredProp === 'suggesterEndpoint') {
+          if (suggesterEndpoint.length == 0) {
+            console.log(route.title + " suggesterEndpoint doesn't have data");


Could you remove this line, please?

ke4 · 2024-09-10T15:24:30Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

+          if (suggesterEndpoint.length == 0) {
+            console.log(route.title + " suggesterEndpoint doesn't have data");
+            shouldHideTab = true
+            return false;


Is it not true?

ke4 · 2024-09-10T15:26:14Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

+
+    function shouldHideMarkerGenesTab(route, props) {
+      const requiredProps = innerTabValidations.get(route.title);
+      let shouldHideTab = false;


We don't need this shouldHideTab variable.
If we got to line 342 (if (shouldHideTab == false)), then this variable is false anyway as nothing changed it from its initial value.
You can remove it without any effect.
Thanks

ke4 · 2024-09-10T15:26:58Z

app/src/main/javascript/bundles/experiment-page/src/TSnePlotViewRoute.js

+          }
+        }
+      });
+      console.log("Marker genes tab: " + shouldHideTab)


Could you remove this line, please?

Signed-off-by: upendrakumbham <[email protected]>

…validate-before-displaying

Initial commit on decoupling tabs in the experiment page

f0677d3

upendrakumbham self-assigned this Dec 12, 2023

upendrakumbham added 2 commits December 12, 2023 16:42

Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…

a21e6ee

…validate-before-displaying

Add multiple ternary conditional operator

7a48932

upendrakumbham requested a review from lingyun1010 January 9, 2024 13:50

upendrakumbham added the scxa label Jan 9, 2024

Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…

a071970

…validate-before-displaying

upendrakumbham linked an issue Jan 10, 2024 that may be closed by this pull request

None of the tabs appearing on the experiment page when plotTypesAndOptions empty in the content payload #362

Open

Improved version of code after a tweak of tab conditions

aa55ced

ke4 reviewed Jan 18, 2024

View reviewed changes

Add fubctions to validate tabs and it's props

585972d

ke4 reviewed Jan 31, 2024

View reviewed changes

upendrakumbham added 2 commits February 2, 2024 13:54

Remove compilation errors and improve functions

25ef163

Remove unused code and improve function

13e7ee2

upendrakumbham marked this pull request as ready for review February 13, 2024 10:32

ke4 requested changes Feb 15, 2024

View reviewed changes

upendrakumbham added 4 commits February 19, 2024 07:45

remove hardcode strings and reuse code

532389c

Add tabtype 'resources' conditional check

1f9aa46

Add empty object check and null, undefined function and update tab va…

2c05e1a

…lidations

Fix camel casing in the function

ca946ac

lingyun1010 closed this Feb 20, 2024

lingyun1010 reopened this Feb 20, 2024

lingyun1010 requested changes Feb 20, 2024

View reviewed changes

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js Outdated Show resolved Hide resolved

ke4 requested changes Feb 20, 2024

View reviewed changes

app/src/main/javascript/bundles/experiment-page/src/ExperimentPageRouter.js Outdated Show resolved Hide resolved

upendrakumbham added 2 commits June 6, 2024 15:01

Initiate tabTypeComponent = []; before return statement to clear data

ea37496

Remove log messages

fcaab7c

upendrakumbham added 16 commits June 26, 2024 13:03

Add validation fields (common & specific)for the tabs

dcf06f8

Add validations for tabs and common props

e84663d

Remove renamed file

08588ac

Signed-off-by: upendrakumbham <[email protected]>

Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…

bef7792

…validate-before-displaying

result tab validations

a60205f

Signed-off-by: upendrakumbham <[email protected]>

conflict resolved html demo page update

53c0ce8

Signed-off-by: upendrakumbham <[email protected]>

add a suggesterEndpoint validation to the result tab

7189295

Signed-off-by: upendrakumbham <[email protected]>

add experiment-design tab validations

d4179e1

Signed-off-by: upendrakumbham <[email protected]>

Add validation attributes to the 'Supplementary-information' and 'Dow…

4d3abc1

…nload' tabs Signed-off-by: upendrakumbham <[email protected]>

Common validations code improvements

bf01724

Signed-off-by: upendrakumbham <[email protected]>

Add inner tab validations and make it result tab enablement depends o…

21731ac

…n any one of the inner tabs visibility Signed-off-by: upendrakumbham <[email protected]>

Update TabConfig

865c176

Signed-off-by: upendrakumbham <[email protected]>

Refactor a function name

fea148e

Signed-off-by: upendrakumbham <[email protected]>

Improve cell plots tab validations function

707dd79

Signed-off-by: upendrakumbham <[email protected]>

Improve cell plots tab and marker genes tab validations

0473eeb

Signed-off-by: upendrakumbham <[email protected]>

Add backend ksWithMarkerGenes attribute payload for the marker genes …

a690dfc

…tab validation of Annadata experiment Signed-off-by: upendrakumbham <[email protected]>

lingyun1010 requested changes Sep 9, 2024

View reviewed changes

ke4 requested changes Sep 10, 2024

View reviewed changes

Remove console.log messages and ; to make code consistent

4ef6772

Signed-off-by: upendrakumbham <[email protected]>

lingyun1010 requested changes Sep 10, 2024

View reviewed changes

Made code format changes to make code consitent

10a0f14

Signed-off-by: upendrakumbham <[email protected]>

ke4 requested changes Sep 10, 2024

View reviewed changes

upendrakumbham added 3 commits September 11, 2024 11:03

Delete propTypes.js file

76ed29e

Signed-off-by: upendrakumbham <[email protected]>

Refactor variable name

f9a1240

Signed-off-by: upendrakumbham <[email protected]>

Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…

2887bfb

…validate-before-displaying

ke4 added bug Something isn't working and removed scxa labels Oct 15, 2024


		const [enableResultTab, setEnableResultTab] = useState(false)


		export const isEmptyArray = (value) => isObjectEmpty(value) && Array.isArray(value);

		export const tabValidations = new Map([

Decoupling tabs in the experiment page and display them after validating #375

Are you sure you want to change the base?

Decoupling tabs in the experiment page and display them after validating #375

Conversation

upendrakumbham commented Dec 12, 2023 • edited Loading

lingyun1010 commented Jan 9, 2024

upendrakumbham commented Jan 9, 2024

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

upendrakumbham commented Feb 13, 2024

ke4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ke4 Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingyun1010 commented Feb 20, 2024 • edited Loading

lingyun1010 left a comment

Choose a reason for hiding this comment

ke4 left a comment

Choose a reason for hiding this comment

lingyun1010 left a comment

Choose a reason for hiding this comment

ke4 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lingyun1010 commented Sep 10, 2024 • edited Loading

lingyun1010 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

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

upendrakumbham commented Sep 10, 2024

upendrakumbham commented Sep 10, 2024

ke4 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

Choose a reason for hiding this comment

upendrakumbham commented Dec 12, 2023 •

edited

Loading

ke4 Feb 15, 2024 •

edited

Loading

lingyun1010 commented Feb 20, 2024 •

edited

Loading

ke4 left a comment •

edited

Loading

lingyun1010 commented Sep 10, 2024 •

edited

Loading