Skip to content
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

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

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f0677d3
Initial commit on decoupling tabs in the experiment page
upendrakumbham Dec 12, 2023
a21e6ee
Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…
upendrakumbham Dec 12, 2023
7a48932
Add multiple ternary conditional operator
upendrakumbham Jan 9, 2024
a071970
Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…
upendrakumbham Jan 9, 2024
aa55ced
Improved version of code after a tweak of tab conditions
upendrakumbham Jan 17, 2024
585972d
Add fubctions to validate tabs and it's props
upendrakumbham Jan 30, 2024
25ef163
Remove compilation errors and improve functions
upendrakumbham Feb 2, 2024
13e7ee2
Remove unused code and improve function
upendrakumbham Feb 9, 2024
532389c
remove hardcode strings and reuse code
upendrakumbham Feb 19, 2024
1f9aa46
Add tabtype 'resources' conditional check
upendrakumbham Feb 19, 2024
2c05e1a
Add empty object check and null, undefined function and update tab va…
upendrakumbham Feb 19, 2024
ca946ac
Fix camel casing in the function
upendrakumbham Feb 19, 2024
ea37496
Initiate tabTypeComponent = []; before return statement to clear data
upendrakumbham Jun 6, 2024
fcaab7c
Remove log messages
upendrakumbham Jun 6, 2024
05d2fb2
Improved code by removing if, else conditions & removed redundant code
upendrakumbham Jun 7, 2024
35bceb9
Merge branch 'develop' into bugfix/decouple-experiment-page-tabs-and-…
upendrakumbham Jun 7, 2024
99088cf
Move all helper functions to tabConfig.js file as part of ExperiemntP…
upendrakumbham Jun 10, 2024
b1cf7e5
Move PropTypes to propTypes.js file as part of ExperiemntPageRouter.j…
upendrakumbham Jun 10, 2024
5eaf719
Improved file
upendrakumbham Jun 10, 2024
dcf06f8
Add validation fields (common & specific)for the tabs
upendrakumbham Jun 26, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from 'react'
import PropTypes from 'prop-types'
import { BrowserRouter, Route, Redirect, Switch, NavLink, withRouter } from 'react-router-dom'
import {BrowserRouter, Route, Redirect, Switch, NavLink, withRouter} from 'react-router-dom'

import URI from 'urijs'

Expand All @@ -10,130 +10,169 @@ import SupplementaryInformationRoute from './SupplementaryInformationRoute'
import DownloadsRoute from './DownloadsRoute'

const RoutePropTypes = {
match: PropTypes.object.isRequired,
location: PropTypes.object.isRequired,
history: PropTypes.object.isRequired
match: PropTypes.object.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

location: PropTypes.object.isRequired,
history: PropTypes.object.isRequired
}

const TabCommonPropTypes = {
atlasUrl: PropTypes.string.isRequired,
experimentAccession: PropTypes.string.isRequired,
species: PropTypes.string.isRequired,
accessKey: PropTypes.string,
resourcesUrl: PropTypes.string
atlasUrl: PropTypes.string.isRequired,
experimentAccession: PropTypes.string.isRequired,
species: PropTypes.string.isRequired,
accessKey: PropTypes.string,
resourcesUrl: PropTypes.string
}

// What component each tab type should render, coupled to ExperimentController.java
const tabTypeComponent = {
'results' : TSnePlotViewRoute,
'experiment-design' : ExperimentDesignRoute,
'supplementary-information' : SupplementaryInformationRoute,
'downloads' : DownloadsRoute
}
let tabTypeComponent = []

const TopRibbon = ({tabs, routeProps}) =>
<ul className={`tabs`}>
{
tabs.map((tab) =>
<li title={tab.name} key={tab.type} className={`tabs-title`}>
<NavLink to={{pathname:`/${tab.type}`, search: routeProps.location.search, hash: routeProps.location.hash}}
activeClassName={`active`}>
{tab.name}
</NavLink>
</li>
)}
</ul>
function isThisTabType(tab, tabType) {
return tab.type === tabType;
}

const isObjectEmpty = (objectName) => {
return (
objectName &&
Object.keys(objectName).length === 0 &&
objectName.constructor === Object
);
};

const tabTypes = [
{ type: 'results', key: 'ks', component: TSnePlotViewRoute, optionsKey: 'plotTypesAndOptions' },
{ type: 'experiment-design', key: 'table.data', component: ExperimentDesignRoute },
{ type: 'supplementary-information', key: 'sections', component: SupplementaryInformationRoute },
{ type: 'resources', key: 'data', component: DownloadsRoute }
];

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const enableExperimentPageTab = (tab) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for (let { type, key, component, optionsKey } of tabTypes) {
if (isThisTabType(tab, type)) {
const propValue = getNestedProperty(tab.props, key);
const optionsValue = optionsKey ? getNestedProperty(tab.props, optionsKey) : true;

if (isNonEmptyArray(propValue) && (!optionsKey || !isObjectEmpty(optionsValue))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tabTypeComponent.push({ [type]: component });
return tab.name;
}
}
}
return null;
};

const TopRibbon = ({tabs, routeProps}) => {
tabTypeComponent = [];
return <ul className={`tabs`}>
{
tabs.map((tab) => (
<li title={tab.name} key={tab.type} className={`tabs-title`}>
<NavLink to={{
pathname: `/${tab.type}`,
search: routeProps.location.search,
hash: routeProps.location.hash
}}
activeClassName={`active`}>
{ enableExperimentPageTab(tab) }
</NavLink>
</li>))
}
</ul>
}
TopRibbon.propTypes = {
tabs: PropTypes.arrayOf(PropTypes.shape({
type: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
props: PropTypes.object
})).isRequired,
routeProps: PropTypes.shape(RoutePropTypes)
tabs: PropTypes.arrayOf(PropTypes.shape({
type: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
props: PropTypes.object
})).isRequired,
routeProps: PropTypes.shape(RoutePropTypes)
}


const TabContent = ({type, tabProps, commonProps, routeProps}) => {
// Pass in the search from location
const Tab = tabTypeComponent[type]
// Pass in the search from location
const Tab = tabTypeComponent[type]

return (
Tab ? <Tab {...tabProps} {...commonProps} {...routeProps}/> : null
)
return (
Tab ? <Tab {...tabProps} {...commonProps} {...routeProps}/> : null
)
}

TabContent.propTypes = {
type: PropTypes.string.isRequired,
tabProps: PropTypes.object,
commonProps: PropTypes.shape(TabCommonPropTypes),
routeProps: PropTypes.shape(RoutePropTypes)
type: PropTypes.string.isRequired,
tabProps: PropTypes.object,
commonProps: PropTypes.shape(TabCommonPropTypes),
routeProps: PropTypes.shape(RoutePropTypes)
}

const RedirectWithSearchAndHash = (props) =>
<Redirect to={{ pathname: props.pathname, search: props.location.search, hash: props.location.hash}} />
<Redirect to={{pathname: props.pathname, search: props.location.search, hash: props.location.hash}}/>

RedirectWithSearchAndHash.propTypes = {
pathname: PropTypes.string.isRequired,
location: PropTypes.shape({
search: PropTypes.string.isRequired,
hash: PropTypes.string.isRequired
}).isRequired
pathname: PropTypes.string.isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

location: PropTypes.shape({
search: PropTypes.string.isRequired,
hash: PropTypes.string.isRequired
}).isRequired
}

const RedirectWithLocation = withRouter(RedirectWithSearchAndHash)

const ExperimentPageRouter = ({atlasUrl, resourcesUrl, experimentAccession, species, accessKey, tabs}) => {
const tabCommonProps = {
atlasUrl,
resourcesUrl,
experimentAccession,
species,
accessKey
}

return (
<BrowserRouter
basename={URI(`experiments/${experimentAccession}`, URI(atlasUrl).path()).toString()}>
<div>
<Route
path={`/`}
render={
(routeProps) =>
<TopRibbon
tabs={tabs}
routeProps={routeProps} />
} />
<Switch>
{
tabs.map((tab) =>
<Route
key={tab.type}
path={`/${tab.type}`}
render={
(routeProps) =>
<TabContent
type={tab.type}
tabProps={tab.props}
commonProps={tabCommonProps}
routeProps={routeProps} />
} />
)
}
<RedirectWithLocation pathname={`/${tabs[0].type}`} />
</Switch>
</div>
</BrowserRouter>
)

const tabCommonProps = {
atlasUrl,
resourcesUrl,
experimentAccession,
species,
accessKey
}

return (
<BrowserRouter
basename={URI(`experiments/${experimentAccession}`, URI(atlasUrl).path()).toString()}>
<div>
<Route
path={`/`}
render={
(routeProps) =>
<TopRibbon
tabs={tabs}
routeProps={routeProps}/>
}/>
<Switch>
{
tabs.map((tab) =>
<Route
key={tab.type}
path={`/${tab.type}`}
render={
(routeProps) =>
<TabContent
type={tab.type}
tabProps={tab.props}
commonProps={tabCommonProps}
routeProps={routeProps}/>
}/>
)
}
<RedirectWithLocation pathname={`/${tabs[0].type}`}/>
</Switch>
</div>
</BrowserRouter>
)
}

ExperimentPageRouter.propTypes = {
...TabCommonPropTypes,
tabs: PropTypes.arrayOf(PropTypes.shape({
type: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
props: PropTypes.object.isRequired
})).isRequired
...TabCommonPropTypes,
tabs: PropTypes.arrayOf(PropTypes.shape({
type: PropTypes.string.isRequired,
name: PropTypes.string.isRequired,
props: PropTypes.object.isRequired
})).isRequired
}

export default ExperimentPageRouter
Loading