Query component #107

adufilie · 2016-12-09T19:12:45Z

What? Why?

Replacement query component which is used on the homepage and can also be embedded in other pages for modifying the query.

Checks

Follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
Follows the Airbnb React/JSX Style guide.
Make sure your commit messages end with a Signed-off-by string (this line
can be automatically added by git if you run the git-commit command with
the -s option)

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. GifGrabber.

Notify reviewers

Read our Pull request merging
policy. If you are part of the
cBioPortal organization, notify the approprate team (remove inappropriate):

@cBioPortal/frontend

If you are not part of the cBioPortal organization look at who worked on the
file before you. Please use git blame <filename> to determine that
and notify them here:

alisman · 2016-12-12T02:28:12Z

src/appShell/App/App.tsx

      }
    }
  }

  render () {
     return (
       <Provider store={this.props.store}>
-         <div style={{ height: '100%' }}>
+         <div style={{ height: 700 }}>


why are we putting a fixed height on this?

This was the easiest way to keep the contents on the screen - had a lot of trouble trying to make bootstrap do that. This change can be reverted.

Yeah, I think this component itself should accept an optional fixed height prop, which would be put on its outermost el.

alisman · 2016-12-12T02:32:29Z

src/appShell/App/Container.tsx

+    	if (children.length == 1)
+    		return children[0];
+
+		return <div style={{width:"100%", height: "100%", flex: 1}}>{children}</div>;


What is the reason for the height, width here?

Because I wanted it to fill the parent. Is there some problem?

A couple things. This div does't actually seem to get rendered because, thus far, there's only every one child. Also, it's not clear to me why manipulating something at the app level would be necessary for the component you are building, which should be self contained and not care about it's surrounding circumstance. Lets remove this unless there's a demonstrable reason for it that I don't understand.

I had to modify this file because the double nested divs that were previously being rendered were interfering with the layout of the child. Line 39 returns the child by itself, which fixes that issue. I added line 41 so the app would display something in the event of unexpected children content. We could throw an error instead if you prefer.

alisman · 2016-12-12T02:34:48Z

src/pages/datasetView/DatasetPage.tsx

@@ -22,10 +24,10 @@ export default class DataSetPageUnconnected extends React.Component<IDatasetPage
        if (this.props.datasets) {
            const rows: Array<any> = [];
            this.props.datasets.forEach((item) => {
-                const tempObj: any = {
+				rows.push({


jiaojiao will replace with unsafe with the custom column component

alisman · 2016-12-12T02:41:27Z

src/pages/patientView/clinicalInformation/SampleLabel.tsx


-export class SampleLabelHTML extends React.Component {
-    constructor(props) {
+export class SampleLabelHTML extends React.Component<ISampleLabelHTMLProps, void> {


curious why is the state interface set to void?

void, {}, null and undefined are pretty much the same

alisman · 2016-12-12T02:48:31Z

src/pages/patientView/patientHeader/PatientHeader.tsx

@@ -40,15 +34,15 @@ export default class PatientHeader extends React.Component<IPatientHeaderProps,
        );
    }

-    private getPopoverPatient(patient: typeof _ClinicalInformationData.patient) {
+    private getPopoverPatient(patient: ClinicalInformationData['patient']) {
        return patient && (


sorry, can you remind me again with this is neccessary? the patient && ...
If patient is falsy, then it will return false, correct? But why is that desirable?

TypeScript settings include strictNullChecks=true and the type of ClinicalInformationData['patient'] includes undefined. If we didn't have this check, it would crash when patient is undefined.

You mean runtime or ts? Is there a legitimate reason for patient to be undefined? If not, it should crash or throw an error. If so, then it should be handled in the control flow. Sorry if I'm misunderstanding.

As I said above, TypeScript settings include strictNullChecks=true and the type of ClinicalInformationData['patient'] includes undefined. This means TypeScript displays an error if you do not check for undefined.

alisman · 2016-12-12T02:51:34Z

src/shared/api/memoize.ts

+	// dual-purpose setter/getter
+	// note: does not work if subsequent calls vary the length of the keys array for the same cache param
+	private traverse(cache:Cache<any, any>, keys:any[], value?:T):T|undefined {
+		if (keys.length == 0)


is there a reason to use == instead of the strict ===?

they are equivalent as long as the length value is a number.

alisman · 2016-12-12T03:18:07Z

src/shared/api/memoize.ts

+/**
+ * Creates a memoized version of a function.
+ */
+export default function memoize<F extends (...args:any[])=>any>(fn:F, getAdditionalMemoizeArgs:()=>any[]):F;


Could we use reselect instead of this? Reselect is documented, tested and widely used. https://github.com/reactjs/reselect. I mean, if there's a good reason to write our own, then by all means.

Reselect only saves the last result (does not cache all results), does not use weak references to parameter values, and has awkward syntax.

alisman · 2016-12-12T15:35:21Z

src/shared/components/query/CancerStudySelector.tsx

+    renderStudyName = (name:string, study:CancerStudy) => {
+    	let checked = !!_.find(this.selectedStudies, id => id == study.cancerStudyIdentifier);
+		return (
+			<Checkbox selected={false} onChange={this.getOnCheckHandler(study)} checked={checked}>


We should but the checkbox in a column of it's own, separate from the name of the study.

I am following the mockup in this RFC:
https://docs.google.com/document/d/1scnR_uxf2Z8X6eqLzwG60rgO4EMtGofMXF0X6oZViaI/edit#

alisman · 2016-12-12T15:41:21Z

src/shared/components/query/CancerStudySelector.tsx

+import {TypeOfCancer, CancerStudy} from "../../api/CBioPortalAPI";
+import {ITreeDescriptor, default as DescriptorTreeNode} from "../tree/DescriptorTreeNode";
+import * as ReactBootstrap from 'react-bootstrap';
+import {BootstrapTable} from "react-bootstrap-table";


I thought we discussed not using react-bootstrap-table? Ironically give it's name, it makes it hard to implement generic bootstrap style. My reasoning is that for simple tables, we want a component that doesn't get away from normal HTML table behavior. This one does because of the way it implements the header cells as a separate table, requiring columns to be of fixed width. Reactable (or MSKReactable) does not do this.

I had already started using it when it was discussed and it is easy to swap out later. I spent minimal time working with the library.

ok. admittedly the table component situation sucks. we're going to have to use FB's fixed data table (in fact our enhancement of it) for tables where performance is a factor because of huge # of rows/columns. it would be nice to keep this to a minimum.

alisman · 2016-12-12T15:47:19Z

src/pages/patientView/clinicalInformation/getClinicalInformationData.ts

-            id: k + '',
-        }))
-        .value();
+    const samples = _.map(


I actually find the initial chained code to be more readable. It's nice to avoid nesting and its nice to have the type right next to the variable we are populating.

Me too but I was changing underscore code to lodash code and did not know the correct way to use chaining.

alisman · 2016-12-12T15:53:18Z

src/shared/components/query/CancerStudySelector.tsx

+	}
+
+    renderStudyName = (name:string, study:CancerStudy) => {
+    	let checked = !!_.find(this.selectedStudies, id => id == study.cancerStudyIdentifier);


as above, the == enables type coercion. I've always understood that === is preferable unless we actually want coercion.

In this case it makes no difference.

alisman · 2016-12-12T15:54:26Z

src/shared/components/query/CancerStudySelector.tsx

+		for (let link of links)
+		{
+			if (children.length)
+				children.push(<span>&nbsp;</span>)


lets use css for padding of this nature

I'm letting @brittanystoroz handle the CSS.

alisman · 2016-12-12T16:00:58Z

src/shared/components/query/styles.module.scss

+  color: black;/*test*/
+}
+
+ul.cancerTypeTree ul {


you could nest these children in the above definition to avoir repetition

Thanks. @brittanystoroz will be writing this file.

alisman · 2016-12-12T16:06:26Z

typings/missing.d.ts

@@ -5,3 +5,4 @@ declare module 'react-file-download';
 declare module 'react-zeroclipboard';
 declare module 'reactableMSK';
 declare module 'redux-seamless-immutable';


does typescript 2.1 with implicit any mean we don't need the missing file anymore? Do you recommend at it be on or off (on by default, right?)?

noImplicitAny is very useful and should remain on, which means we need to declare these modules.

alisman · 2016-12-12T16:13:03Z

src/shared/components/tree/DescriptorTreeNode.tsx

+		this.state = {};
+	}
+
+	onExpand = () =>


is this a way of getting around binding issue?

actually, it looks like this isn't used, right? onExpand seems to come from props

This binding is required because the function is passed to a subcomponent.

alisman · 2016-12-12T16:15:02Z

src/shared/components/tree/DescriptorTreeNode.tsx

+        	<FontAwesome
+        		name={expand ? 'caret-down' : 'caret-right'}
+        		style={arrowStyle}
+        		onMouseDown={this.onExpand}


hmmm. it is used here.

alisman · 2016-12-12T16:17:11Z

src/shared/components/query/CancerStudySelector.tsx

+
+	@memoizeWith(function() { return [this.props.cancerTypes, this.props.studies] })
+    getTreeDescriptor():ITreeDescriptor<TypeOfCancer|CancerStudy> & {rootNode: TypeOfCancer, setExpanded: (node:Node, value:boolean) => void} {
+		let {


some inconsistent indentation in this file. that is one linting task i think is important. otherwise things get very unreadable fast. it's important that people be able to use the IDE reformat and that it aligns with linting

indentation already addressed in latest commit

I am fine having a script that normalizes the indentation but it is anti-productive to worry about it while writing code.

alisman · 2016-12-12T16:47:27Z

src/globalStyles/global.scss

+
+/* makes react-bootstrap-table work in flexbox layout */
+.react-bs-table-container,.react-bs-table,.react-bs-container-body {


Putting aside whether we use react-bs-table or not, this would be better applied to the component instance itself.

i will move this to a better location

alisman · 2016-12-12T17:23:25Z

src/appShell/App/Container.tsx

+
+        // don't create extra div elements if there is only one child
+        if (children.length == 1)
+            return children[0];


We just need to set this back to what it was. I don't think there is a reason that nested parent divs should cause a problem for the layout you are trying to achieve. I'm happy to help address the underlying layout issue.

Why do we need to change this back? The new code is better. Why do you want two divs surrounding the child?

There shouldn't sometimes be a wrapping div at this level and not other times. If there's only ever one child then lets just change the code. More than anything else though, the fact that a change was made here is a flag that there is a deeper confusion. Instead of changing something global, lets try to resolve that local confusion.

Like I said, we can throw an error when there is more than one child if you prefer.

Here's a screenshot of what it shows with two wrapping divs: http://image.prntscr.com/image/7ef4209fefca47f88ce4b8ab5187c694.png

Here's a screenshot of what it shows with no wrapping div: http://image.prntscr.com/image/a966d92465154f93996fd90f6d39d8e9.png

Remove DevMode code

Added options to explicitly set selected values and customize label

More changes for patient view mutations tab

adufilie added the DO NOT MERGE label Dec 9, 2016

adufilie assigned adufilie, alisman, brittanydionigi and inodb Dec 9, 2016

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 9, 2016 19:14 Inactive

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 9, 2016 21:51 Inactive

alisman reviewed Dec 12, 2016

View reviewed changes

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 12, 2016 16:02 Inactive

alisman reviewed Dec 12, 2016

View reviewed changes

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 12, 2016 19:04 Inactive

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 12, 2016 19:34 Inactive

jjgao temporarily deployed to cbioportal-frontend-pr-107 December 12, 2016 19:46 Inactive

brittanydionigi and others added 5 commits April 24, 2017 14:55

Background and borders

34ec7db

Containers

4c22fd9

Consistent error styling and margins

3a4e77a

Remove temporary hacks

e02c479

Fix performance issue in cancer study selector

a171a98

adufilie force-pushed the homepage branch from e70195f to a171a98 Compare April 24, 2017 19:11

jjgao temporarily deployed to cbioportal-frontend-pr-107 April 24, 2017 19:12 Inactive

Move selected studies view to modal dialog

340746f

Remove DevMode code

jjgao temporarily deployed to cbioportal-frontend-pr-107 April 24, 2017 20:41 Inactive

alisman closed this May 19, 2017

alisman reopened this May 24, 2017

jjgao temporarily deployed to cbioportal-frontend-pr-107 May 24, 2017 22:05 Inactive

alisman closed this May 25, 2017

onursumer added a commit to onursumer/cbioportal-frontend that referenced this pull request Dec 18, 2019

Merge pull request cBioPortal#107 from onursumer/selected-values-option

9106e4a

Added options to explicitly set selected values and customize label

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 11, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

b26ce8f

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 16, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

74297bf

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 18, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

63436bd

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 19, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

bdfa788

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 20, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

bcfc0b6

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 23, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

ab4ff85

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 24, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

eabb673

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 25, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

0ccd6bb

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 26, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

f0d8046

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

9a10723

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

774162b

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

eaed1cd

More changes for patient view mutations tab

alisman pushed a commit to alisman/cbioportal-frontend that referenced this pull request Jun 2, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

e46b3db

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

9eaa7c7

More changes for patient view mutations tab

onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

d4c2de6

More changes for patient view mutations tab

alisman pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 8, 2022

Merge pull request cBioPortal#107 from cBioPortal/more-changes

34d3908

More changes for patient view mutations tab


		/* makes react-bootstrap-table work in flexbox layout */
		.react-bs-table-container,.react-bs-table,.react-bs-container-body {

Query component #107

Query component #107

Conversation

adufilie commented Dec 9, 2016 • edited Loading

What? Why?

Checks

Any screenshots or GIFs?

Notify reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisman Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisman Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisman Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adufilie Dec 12, 2016 • edited Loading

Choose a reason for hiding this comment

adufilie commented Dec 9, 2016 •

edited

Loading

alisman Dec 12, 2016 •

edited

Loading

alisman Dec 12, 2016 •

edited

Loading

alisman Dec 12, 2016 •

edited

Loading

adufilie Dec 12, 2016 •

edited

Loading