Implement RFC85 Dynamic Virtual Study #5016
Conversation
Added option to select a dynamic type of virtual study while creating it Static type (the legacy one) is the default one
✅ Deploy Preview for cbioportalfrontend ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
To make it follow the conventions and escape confusion with java bean property convenction during deserialisation on BE
There was a problem hiding this comment.
Thanks @forus !
I added some proposals to improve the tooltip text.
Co-authored-by: pieterlukasse <pieterlukasse@gmail.com>
|
@alisman @haynescd @pieterlukasse I've changed code to provide different descriptions for static and dynamic Virtual Study by default. Check 4b49f4f |
| this.props.caseListNameSet, | ||
| this.props.user | ||
| ); | ||
| this.setDynamicTypeTo(false); |
There was a problem hiding this comment.
maybe a more "react" correct way would be to have dynamic boolean variable be a state and let the description react to that.
There was a problem hiding this comment.
@pieterlukasse do you have an example in the code of this approach?
There was a problem hiding this comment.
i think @pieterlukasse is right. it seems you want to keep dynamic as a observable state and then make the rest a computation which depends upon it. you will see the @computed decorator used thoughout our codebase and that is what it accomplishes. any getter that is tagged as computed will recompute whenever the observable state which it references changes.
There was a problem hiding this comment.
@alisman @pieterlukasse Please check 146911a
forus
force-pushed
the
rfc85-dynamic-virtual-study
branch
2 times, most recently
from
0f198c6
to
846b9d4
Compare
forus
force-pushed
the
rfc85-dynamic-virtual-study
branch
from
7871b78
to
146911a
Compare
| ); | ||
| } | ||
|
|
||
| function getResourceDataOfPatients(studyClinicalData: { | ||
| [uniqueSampleKey: string]: ClinicalData[]; | ||
| }) { | ||
| const resourcesPerPatient = _(studyClinicalData) | ||
| .flatMap(clinicaldataItems => clinicaldataItems) | ||
| .flatMap((clinicaldataItems) => clinicaldataItems) |
There was a problem hiding this comment.
seems like you maybe have a code linter that is reformatting things? would rather not introduce all this change.
There was a problem hiding this comment.
I get this file changed by yarn run prettierFixLocal command although I haven't change this one. I can revert prettifications for files I did not change. How does it sound?
| return this.customDescription || ''; | ||
| } | ||
|
|
||
| set description(description: string) { |
There was a problem hiding this comment.
you shouldn't have a setter. the description get above can check the prop if there is one. i actually don't see where this called
There was a problem hiding this comment.
See onChange handler for the descriptions field:
onChange={event =>
(this.description =
event.currentTarget.value)
}
yea, the setter is not necessary. We can use the customDescription like the following and get rid from the setter:
onChange={event =>
(this.customDescription =
event.currentTarget.value)
}
Is that what you meant?
There was a problem hiding this comment.
yes. that would be cleaner. i just don't like there be a setter on a property which is computed because it can create confusion.
Fix cBioPortal/cbioportal# (see https://help.github.com/en/articles/closing-issues-using-keywords)
Describe changes proposed in this pull request:
Checks
Any screenshots or GIFs?
If this is a new visual feature please add a before/after screenshot or gif
here with e.g. Giphy CAPTURE or Peek
Notify reviewers
Read our Pull request merging
policy. It can help to figure out who worked on the
file before you. Please use
git blame <filename>to determine thatand notify them either through slack or by assigning them as a reviewer on the PR