-
Notifications
You must be signed in to change notification settings - Fork 5
Decoupling tabs in the experiment page and display them after validating #375
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
base: develop
Are you sure you want to change the base?
Changes from all commits
f0677d3
a21e6ee
7a48932
a071970
aa55ced
585972d
25ef163
13e7ee2
532389c
1f9aa46
2c05e1a
ca946ac
ea37496
fcaab7c
05d2fb2
35bceb9
99088cf
b1cf7e5
5eaf719
dcf06f8
e84663d
08588ac
bef7792
a60205f
53c0ce8
7189295
d4179e1
4d3abc1
bf01724
21731ac
865c176
fea148e
707dd79
0473eeb
a690dfc
4ef6772
10a0f14
76ed29e
f9a1240
2887bfb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React from 'react' | ||
| import React, { useState } from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import { BrowserRouter, Route, Redirect, Switch, NavLink, withRouter } from 'react-router-dom' | ||
|
|
||
|
|
@@ -8,132 +8,221 @@ import TSnePlotViewRoute from './TSnePlotViewRoute' | |
| import ExperimentDesignRoute from './ExperimentDesignRoute' | ||
| import SupplementaryInformationRoute from './SupplementaryInformationRoute' | ||
| import DownloadsRoute from './DownloadsRoute' | ||
| import {isEmptyArray, tabCommonValidations, tabValidations} from "./TabConfig" | ||
|
|
||
| const RoutePropTypes = { | ||
| match: PropTypes.object.isRequired, | ||
| location: PropTypes.object.isRequired, | ||
| history: PropTypes.object.isRequired | ||
| match: PropTypes.object.isRequired, | ||
| 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, | ||
|
Contributor
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. You already externalised the |
||
| 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 | ||
| 'results' : TSnePlotViewRoute, | ||
| 'experiment-design' : ExperimentDesignRoute, | ||
| 'supplementary-information' : SupplementaryInformationRoute, | ||
| 'downloads' : DownloadsRoute | ||
| } | ||
|
|
||
| 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 shouldRenderTab(tab, commonProps) { | ||
| const commonRequiredProps = tabCommonValidations.get(tab.type) | ||
| let shouldRender = true | ||
|
|
||
| if (commonRequiredProps != null) { | ||
| commonRequiredProps.some(commonProp => { | ||
| 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 | ||
| return false | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| const requiredProps = tabValidations.get(tab.type) | ||
| const tabProps = tab.props | ||
|
|
||
| if (requiredProps != null) { | ||
| requiredProps.forEach(requiredProp => { | ||
| // Check if property requires nested object validation | ||
| if (requiredProp.includes('.')) { | ||
| const splitProps = requiredProp.split('.') | ||
| splitProps.forEach(splitProp => { | ||
|
|
||
| let table = [] | ||
| let tableHeader = [] | ||
| let tableData = [] | ||
|
|
||
| if (isEmptyArray(table)) { | ||
|
Contributor
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. As you just defined |
||
| table = tabProps[splitProp] || [] | ||
| if (table.length == 0) { | ||
| console.log(`${tab.type}: table doesn't have data`) | ||
| shouldRender = false | ||
| return false // Early return on failure | ||
| } | ||
| } | ||
|
|
||
| if (isEmptyArray(tableHeader)) { | ||
|
Contributor
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. The same as with the |
||
| tableHeader = table[splitProp] || [] | ||
| if (tableHeader.length == 0) { | ||
| console.log(tab.type + ":" + " table headers doesn't have data") | ||
| shouldRender = false | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| if (isEmptyArray(tableData)) { | ||
| tableData = tableHeader[splitProp] | ||
| if (tableData.length == 0) { | ||
| console.log(tab.type + ":" + " table data doesn't have") | ||
| shouldRender = false | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| }) | ||
| return shouldRender | ||
| } | ||
| }) | ||
| } | ||
| return shouldRender | ||
| } | ||
|
|
||
| const TopRibbon = ({tabs, routeProps, commonProps}) => | ||
| <ul className={`tabs`}> | ||
| { | ||
| tabs.map((tab) => { | ||
| if(shouldRenderTab(tab, commonProps) === true) { | ||
| return <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> | ||
|
|
||
| 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({ | ||
|
Contributor
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. You can move all the propTypes from this class to the |
||
| 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] | ||
| const TabContent = ({type, tabProps, commonProps, routeProps, resultTabView}) => { | ||
| // Pass in the search from location | ||
| const Tab = tabTypeComponent[type] | ||
|
|
||
| return ( | ||
| Tab ? <Tab {...tabProps} {...commonProps} {...routeProps}/> : null | ||
| ) | ||
| return ( | ||
| Tab ? <Tab {...tabProps} {...commonProps} {...routeProps} enableView={resultTabView} /> : 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), | ||
| resultTabView : PropTypes.func | ||
| } | ||
|
|
||
| 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, | ||
|
Contributor
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. The
Contributor
Author
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. I agree with you, I will try to do that.
Contributor
Author
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.
This one is fixed. Please have a look.
Contributor
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. I don't know what you meant by |
||
| 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 [enableResultTab, setEnableResultTab] = useState(false) | ||
|
|
||
| let resultTabView = (resultTabView) => { | ||
| if(resultTabView) { | ||
| setEnableResultTab(true) | ||
| } | ||
| } | ||
|
|
||
| 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} | ||
| commonProps={tabCommonProps} | ||
| /> | ||
| } /> | ||
| <Switch> | ||
| { | ||
| tabs.map((tab) => | ||
| { | ||
| if(shouldRenderTab(tab, tabCommonProps)) { | ||
| return <Route | ||
| key={tab.type} | ||
| path={`/${tab.type}`} | ||
| render={ | ||
| (routeProps) => | ||
| <TabContent | ||
| type={tab.type} | ||
| tabProps={tab.props} | ||
| commonProps={tabCommonProps} | ||
| routeProps={routeProps} | ||
| resultTabView={resultTabView} | ||
| /> | ||
| }/> | ||
| } | ||
| }) | ||
| } | ||
| <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 | ||
| export default ExperimentPageRouter | ||
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.
To clean the code and make this file smaller, I would externalise all the
proptypesto another file.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.
Could you group together the code parts that belongs to
PropTypes?Thanks