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

…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

ke4 · 2024-06-06T16:01:42Z

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

+    const supplementaryInformationTab = 'supplementary-information'
+    const downloadTab = 'resources'
+
+    if (isThisTabType(tab, resultTab)) {


I would like you to change/refactor this conditional if statement and NOT using any conditional statements at all.
There is a much better way to do it and the code would be much sorter and cleaner.
Thanks

Sure, I will improve it.

ke4 · 2024-06-06T16:03:31Z

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

+    const downloadTab = 'resources'
+
+    if (isThisTabType(tab, resultTab)) {
+        if (!isObjectEmpty(tab.props.ks) && Array.isArray(tab.props.ks) && !isObjectEmpty(tab.props.plotTypesAndOptions)) {


I would suggest to add these kind of checks to the prop types definition.
That is true in the below lines, too.
Thanks

ke4

I added 2 requests that I would like you to change to make the code cleaner.
Many thanks

…validate-before-displaying

upendrakumbham · 2024-06-07T13:21:04Z

Hi both, I improved the code. Please re-review and add your comments.

ke4

I added some comments. Could you please review and consider them?
Thanks

ke4 · 2024-06-07T15:21:30Z

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,


To clean the code and make this file smaller, I would externalise all the proptypes to another file.

ke4 · 2024-06-07T15:24:15Z

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

+
+const getNestedProperty = (obj, path) => path.split('.').reduce((acc, key) => acc?.[key], obj);
+
+const isNonEmptyArray = (value) => !isObjectEmpty(value) && Array.isArray(value);


Should we move this code: Array.isArray(value) into its relevant proptype definition?

This is one of the validation checks before displaying a relevant tab. We are getting empty for some experiments.

ke4 · 2024-06-07T15:26:15Z

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

+            const propValue = getNestedProperty(tab.props, key);
+            const optionsValue = optionsKey ? getNestedProperty(tab.props, optionsKey) : true;
+
+            if (isNonEmptyArray(propValue) && (!optionsKey || !isObjectEmpty(optionsValue))) {


What happens in isObjectEmpty(optionsValue) if the optionsValue is true?

ke4 · 2024-06-07T15:28:42Z

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

+
+const isNonEmptyArray = (value) => !isObjectEmpty(value) && Array.isArray(value);
+
+const enableExperimentPageTab = (tab) => {


This whole function is a bit too complex for me and hard to understand what is happening inside this function. It is really hard to follow.
I am happy to work together on this code and make it more simpler, more understandable.
What do you think?

Ok, we will. Thanks. We will do Monday. I will try to improve variable naming.

ke4 · 2024-06-07T15:30:38Z

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

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


The proptypes are all over this JS file.
As I already mentioned, I would externalise them to a separate file and import them into this one.
That would make this file more clear.
What do you think?

I agree with you, I will try to do that.

The proptypes are all over this JS file.
As I already mentioned, I would externalise them to a separate file and import them into this one.
That would make this file more clear.
What do you think?

This one is fixed. Please have a look.

…ageRouter.js file cleanup

…s file cleanup to reduce it's size for the future maintenance

lingyun1010 · 2024-06-11T13:42:11Z

Good work and I have a few comments listed below:

Code format: we do not use ; to end any javascript code line
In the current implementation, we only check one key and one optionsKey, firstly can you explain what is the difference between these two props? Are they required and non-required? As far as I know, we have more than one required props at least for the Result tab. Please double check the other tabs and add more possible checks.
Given the above issue solved, we should remove the corresponding top ribbon fully, not only the ribbon title, here is a screenshot while I tested locally by removing ks from result tab, which may give you some clues about my comment.

lingyun1010

In the current implementation, you are just considering decoupling from the top ribbon tabs, i.e., Results, Supplementary Infomation and Download, but I think we should also consider to decouple the tabs in Results page, i.e. Cell-plots, Maker Genes and Anatomogram. If one of the side-tabs has missing props, we should still keep the others and leave Results top tab alive. If all the side tabs have missing props, then we remove the Results top tab as you implemented now.

upendrakumbham · 2024-06-24T12:45:17Z

Hi @lingyun1010, gathered all the required validation fields to validate and display their corresponding tabs.

Here is the document: Frontend_tab_validations.docx

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

ke4 reviewed Jun 6, 2024

View reviewed changes

ke4 requested changes Jun 6, 2024

View reviewed changes

upendrakumbham added 2 commits June 7, 2024 14:14

Improved code by removing if, else conditions & removed redundant code

05d2fb2

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

35bceb9

…validate-before-displaying

upendrakumbham requested review from ke4 and lingyun1010 June 7, 2024 13:21

ke4 requested changes Jun 7, 2024

View reviewed changes

upendrakumbham added 3 commits June 10, 2024 16:10

Move all helper functions to tabConfig.js file as part of ExperiemntP…

99088cf

…ageRouter.js file cleanup

Move PropTypes to propTypes.js file as part of ExperiemntPageRouter.j…

b1cf7e5

…s file cleanup to reduce it's size for the future maintenance

Improved file

5eaf719

lingyun1010 requested changes Jun 12, 2024

View reviewed changes

Add validation fields (common & specific)for the tabs

dcf06f8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

upendrakumbham commented Dec 12, 2023

lingyun1010 commented Jan 9, 2024

upendrakumbham commented Jan 9, 2024

ke4 Jan 18, 2024

upendrakumbham Jan 30, 2024

upendrakumbham Jan 30, 2024

ke4 Jan 31, 2024

upendrakumbham commented Feb 13, 2024

ke4 left a comment

ke4 Feb 15, 2024

ke4 Feb 15, 2024 •

edited

Loading

upendrakumbham Feb 16, 2024

lingyun1010 commented Feb 20, 2024 •

edited

Loading

lingyun1010 left a comment

ke4 left a comment

ke4 Jun 6, 2024

upendrakumbham Jun 6, 2024

ke4 Jun 6, 2024

ke4 left a comment

upendrakumbham commented Jun 7, 2024

ke4 left a comment

ke4 Jun 7, 2024

ke4 Jun 7, 2024

upendrakumbham Jun 7, 2024

ke4 Jun 7, 2024

ke4 Jun 7, 2024

upendrakumbham Jun 7, 2024

ke4 Jun 7, 2024

upendrakumbham Jun 7, 2024

upendrakumbham Jun 10, 2024

lingyun1010 commented Jun 11, 2024 •

edited

Loading

lingyun1010 left a comment

upendrakumbham commented Jun 24, 2024


		const getNestedProperty = (obj, path) => path.split('.').reduce((acc, key) => acc?.[key], obj);

		const isNonEmptyArray = (value) => !isObjectEmpty(value) && Array.isArray(value);


		const isNonEmptyArray = (value) => !isObjectEmpty(value) && Array.isArray(value);

		const enableExperimentPageTab = (tab) => {

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ke4 left a comment

Choose a reason for hiding this comment

upendrakumbham commented Jun 7, 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

lingyun1010 commented Jun 11, 2024 • edited Loading

lingyun1010 left a comment

Choose a reason for hiding this comment

upendrakumbham commented Jun 24, 2024

ke4 Feb 15, 2024 •

edited

Loading

lingyun1010 commented Feb 20, 2024 •

edited

Loading

lingyun1010 commented Jun 11, 2024 •

edited

Loading