combined gwas step2 #1124
combined gwas step2 #1124
Conversation
alilahrime
requested review from
pieterlukasse,
jarvisraymond-uchicago and
nmetokishlubsky
There was a problem hiding this comment.
hi @alilahrime ,
thanks for the PR! I added two comments which I think need to be addressed. Can you please take a look?
| ); | ||
| case 2: | ||
| // covariates (customdichtomous or not) | ||
| return ( |
There was a problem hiding this comment.
this step is about outcome phenotype (selecting covariates is only done in next step). I propose we change the naming to reflect this as much as possible.
| sourceId={sourceId} | ||
| // searchTerm={searchTerm} | ||
| /> | ||
| <button type='button' style={{ marginLeft: 5 }} onClick={() => setMode('')}>Submit</button> |
There was a problem hiding this comment.
the current code returns to the start of step2 when submit is clicked. Instead, it should go to step3 once a selection is submitted, given that we can have only 1 outcome phenotype.
There was a problem hiding this comment.
@alilahrime Great work here! Lots of new functionality! I ran your code locally and it seems to work real good. I left lots of questions as I'm still learning the codebase and standards here: please let me know. Thank you!
Also: I'm not clear on when we are adding Storybooks. Should this PR contain Storybooks? Should a separate ticket be raised to add storybooks?
| @@ -13,7 +13,7 @@ import { analysisApps } from '../localconf'; | |||
| import './AnalysisApp.css'; | |||
| import sessionMonitor from '../SessionMonitor'; | |||
| import GWASWorkflowList from './GWASUIApp/GWASWorkflowList'; | |||
| import GWASContainer from "./GWASV2/GWASContainer"; | |||
There was a problem hiding this comment.
Question: Correct me if I'm wrong, but I believe this is a case where this file doesn't directly related to one of the tickets, but is included due to eslint formatting?
I was able to revert this using a GIT command:
git checkout origin/master src/Analysis/AnalysisApp.jsx
Do you think it would make it help to create a Spike ticket for this? Is this reoccurring? I was thinking it might be good to add a command like eslint-new to package.json that would only run eslint fixes on files that have already been changed in the branch.
| @@ -2,11 +2,11 @@ import React from 'react'; | |||
| import { Collapse, List, Spin } from 'antd'; | |||
| import './GWASUIApp.css'; | |||
| import { useQuery } from 'react-query'; | |||
| import { gwasWorkflowPath } from '../../localconf'; | |||
There was a problem hiding this comment.
Same as above: Question: Correct me if I'm wrong, but I believe this is a case where this file doesn't directly related to one of the tickets, but is included due to eslint formatting?
I was able to revert this file using:
git checkout origin/master src/Analysis/GWASUIApp/GWASWorkflowList.jsx
|
|
||
| const GWASContainer = () => { | ||
| const [current, setCurrent] = useState(0); | ||
| const [ | ||
| selectedStudyPopulationCohort, | ||
| setSelectedStudyPopulationCohort, | ||
| ] = useState({}); | ||
| const gwasSteps = [ |
There was a problem hiding this comment.
I like this, it makes it easier to read by abstracting this data into a constants file like it was before. 👍
| import SelectStudyPopulation from './SelectStudyPopulation/SelectStudyPopulation'; | ||
| import SelectOutcome from './SelectOutcome/SelectOutcome'; | ||
| import SelectCovariates from './SelectCovariates/SelectCovariates'; | ||
| import './GWASV2.css'; |
There was a problem hiding this comment.
Nitpick: I've been adding CSS imports last for consistency.
| <Space direction={"vertical"} style={{ width: "100%" }}> | ||
| <div className="steps-content"> | ||
| <style> | ||
| {`.analysis-app__actions > div:nth-child(1) { ${current === 0 ? 'margin: 0 auto;' : ''} ${current === 1 || 2 ? 'marginLeft: 0' : ''} }`} |
There was a problem hiding this comment.
Nitpick: I know this isn't in the code style guide, but can we break up long lines? When a line is over 80-100 characters it makes it more difficult to read. For example, you have to scroll horizontally in Github to read the line of code.
| }) => { | ||
| const covariates = useQuery(['covariates', sourceId], () => fetchCovariates(sourceId), queryConfig); | ||
| const fetchedCovariates = useFetch(covariates, 'concepts'); | ||
| // const displayedCovariates = useFilter(fetchedCovariates, searchTerm, 'concept_name'); |
There was a problem hiding this comment.
Can we remove this line of commented out code?
| @@ -0,0 +1,19 @@ | |||
| /* eslint-disable import/prefer-default-export */ | |||
| export const gwasSteps = [ | |||
There was a problem hiding this comment.
I like this idea of using a constants.js file again and have updated my ticket to use this.
If we do keep this file, do you think it would be better organized if placed in the "shared" folder?
There was a problem hiding this comment.
will swap to the place you specified in progress bar pr
| @@ -224,7 +224,7 @@ const DiscoveryDetails = (props: Props) => { | |||
| <div className='discovery-modal__header-buttons'> | |||
| <Button | |||
| type='text' | |||
| onClick={() => { props.setModalVisible(false); setTabActiveKey('0');}} | |||
| onClick={() => { props.setModalVisible(false); setTabActiveKey('0'); }} | |||
There was a problem hiding this comment.
Consider reverting this file.
| @@ -151,7 +151,7 @@ class MapDataModel extends React.Component { | |||
| if (data && data[this.state.parentNodeType]) { | |||
| this.setState((prevState) => ({ validParentIds: data[prevState.parentNodeType] })); | |||
| } | |||
| }).catch(error => { | |||
| }).catch((error) => { | |||
There was a problem hiding this comment.
Consider reverting this file.
| @@ -344,105 +344,105 @@ function buildConfig(opts) { | |||
| analysisTools.forEach((at) => { | |||
| if (typeof at === 'string' || at instanceof String) { | |||
| switch (at) { | |||
| case 'ndhHIV': | |||
There was a problem hiding this comment.
Consider reverting this file.
|
PR replaced by #1141 |
VADC-217
VADC-218
VADC-219
Bug Fixes
hoist source fetch
generalize covariate (continuous vs. custom dichotomous) components to be used for outcome or non-outcome selections
minor placeholder styling
added state variables for currently selected covariate and comments for remaining todos that use them
Dependency Updates
no updates