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

MatchMiner increase column width for clinical criteria #2978

Merged
merged 1 commit into from
Jan 22, 2020

Conversation

victoria34
Copy link
Contributor

@victoria34 victoria34 commented Jan 10, 2020

Fix oncokb/oncokb#1891

Most trials are matched based on both clinical and genomic criteria. However, some of them can be matched only by clinical criteria. I changed the width to 100% instead of 50% for that case.

Screen Shot 2020-01-10 at 4 38 16 PM

@alisman
Copy link
Collaborator

alisman commented Jan 14, 2020

@victoria34 this looks fine to me. we should try to get in the habit of asking product owners (Sarah? Debiani?) to review this in the browser before we merge. They can do this by making the following bookmark in their browser:

javascript:

(function(){var pr = prompt("Please enter PR#");if (pr && Number(pr)) { localStorage.netlify = `deploy-preview-${pr}--cbioportalfrontend`;window.location.reload(); }})()

Then, go the page where they want to see the new UI on the live site and click the button. They will be prompted for a PR number. In this case, 2978. The page will reload and they should then see the changes you made in the context of the production website.

Note that this works only for frontend changes.

@victoria34
Copy link
Contributor Author

victoria34 commented Jan 14, 2020

@victoria34 this looks fine to me. we should try to get in the habit of asking product owners (Sarah? Debiani?) to review this in the browser before we merge. They can do this by making the following bookmark in their browser:
javascript:(function()%7Bvar%20pr%20%3D%20prompt(%22Please%20enter%20PR%23%22)%3Bif%20(pr%20%26%26%20Number(pr))%20%7B%20localStorage.netlify%20%3D%20%60deploy-preview-%24%7Bpr%7D--cbioportalfrontend%60%3Bwindow.location.reload()%3B%20%7D%7D)()

Then, go the page where they want to see the new UI on the live site and click the button. They will be prompted for a PR number. In this case, 2978. The page will reload and they should then see the changes you made in the context of the production website.

Note that this works only for frontend changes.

Thanks for letting me know. This is really helpful to me and Sarah @phillis2. Hi Sarah @phillis2, could you try this when you get a chance. If you have feedback, please leave your comments in here. Thanks

@victoria34
Copy link
Contributor Author

Hi @alisman, I have demoed this to @phillis2 and she is satisfied with the new interface. Please merge the PR when you get a chance. Thanks

@victoria34 victoria34 requested a review from onursumer January 22, 2020 17:00
@victoria34
Copy link
Contributor Author

Hi @alisman, could you review my PR when you get a chance? Thanks

@alisman alisman merged commit 4562d78 into cBioPortal:master Jan 22, 2020
@@ -69,10 +69,10 @@ export function groupTrialMatchesByAgeNumerical(armGroup: ITrialMatch[]): IClini
}
};
const positiveTrialMatches = _.filter(matchesGroupedByAge[age], (trialMatch:ITrialMatch) => {
if (!_.isUndefined(trialMatch.genomicAlteration)) return !trialMatch.genomicAlteration.includes('!')
if (!_.isUndefined(trialMatch.genomicAlteration) && trialMatch.genomicAlteration !== '') return !trialMatch.genomicAlteration.includes('!')
Copy link
Member

Choose a reason for hiding this comment

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

always use brackets after if statements:

Suggested change
if (!_.isUndefined(trialMatch.genomicAlteration) && trialMatch.genomicAlteration !== '') return !trialMatch.genomicAlteration.includes('!')
if (!_.isUndefined(trialMatch.genomicAlteration) && trialMatch.genomicAlteration !== '') {
return !trialMatch.genomicAlteration.includes('!')
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update my code. Thanks

});
const negativeTrialMatches = _.filter(matchesGroupedByAge[age], (trialMatch:ITrialMatch) => {
if (!_.isUndefined(trialMatch.genomicAlteration)) return trialMatch.genomicAlteration.includes('!')
if (!_.isUndefined(trialMatch.genomicAlteration) && trialMatch.genomicAlteration !== '') return trialMatch.genomicAlteration.includes('!')
Copy link
Member

Choose a reason for hiding this comment

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

same as above, always use brackets

@@ -64,7 +64,7 @@
.firstRight{
display:flex;
flex: 0 0 auto;
width: 300px;
width: 50%;
Copy link
Member

Choose a reason for hiding this comment

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

the PR title says 100%, but we set it here to 50%. Are they two unrelated things?

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 default width is 50% but it will be set as 100% when there is no genomic content.

@inodb inodb added cl-style-tweak Style tweak section of changelog cl-external-api External API section of changelog. Changes to handle an external API (i.e. not cBioPortal API) and removed cl-style-tweak Style tweak section of changelog labels Jan 30, 2020
@inodb inodb changed the title Set width as 100% of clinical cretieria container when the trial is matc… MatchMiner increase column width for clinical criteria Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cl-external-api External API section of changelog. Changes to handle an external API (i.e. not cBioPortal API) ready to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The matched link for this trial shows patients, but none of them match to the trial - NCT02631044
4 participants