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

Upgraded react-mutation-mapper version to 0.3.0 #2706

Merged
merged 2 commits into from Nov 19, 2019

Conversation

onursumer
Copy link
Member

@onursumer onursumer commented Sep 17, 2019

Before:
mutations_tab_current

After:
mutation-mapper-multi-filter

Fix cBioPortal/cbioportal/issues/5338

Checks

  • Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!
  • 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)

@onursumer onursumer force-pushed the react-mutation-mapper-0.3.0 branch 3 times, most recently from 2056fe5 to 76bb3ab Compare September 19, 2019 20:43
@@ -130,43 +130,6 @@ export default class MutationMapperStore extends DefaultMutationMapperStore
return mutation.gene.entrezGeneId;
}

@autobind
protected customFilterApplier(filter: DataFilter,
Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -188,18 +201,6 @@ export default class MutationMapper<P extends IMutationMapperProps> extends Defa
): null;
}

protected get proteinImpactTypePanel(): JSX.Element | null
Copy link
Member Author

Choose a reason for hiding this comment

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

now we are using ProteinImpactTypeBadgeSelector component to enable multiple filtering

{this.somaticMutationFrequency()}
{this.germlineMutationFrequency()}
</div>
<MutationStatusSelector
Copy link
Member Author

Choose a reason for hiding this comment

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

This enables filtering by mutation status (somatic / germline)

package.json Outdated Show resolved Hide resolved
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2706 September 19, 2019 21:45 Inactive
@jjgao jjgao temporarily deployed to cbioportal-frontend-pr-2706 September 20, 2019 17:32 Inactive
@alisman
Copy link
Collaborator

alisman commented Oct 4, 2019

@onursumer is this still draft? what's story?

@onursumer
Copy link
Member Author

@alisman we can merge this as soon as Niki approves the latest UI changes. I still need to update the screenshot tests though.

@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr October 23, 2019 18:58 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr October 28, 2019 07:59 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr October 28, 2019 09:21 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr October 31, 2019 20:04 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 1, 2019 10:09 Inactive
@onursumer onursumer marked this pull request as ready for review November 1, 2019 10:16
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 14, 2019 17:26 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 14, 2019 18:24 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 15, 2019 16:51 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 15, 2019 17:23 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 18, 2019 16:50 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 18, 2019 20:07 Inactive
@@ -171,6 +186,10 @@ export default class MutationMapper<P extends IMutationMapperProps> extends Defa
trackDataStatus={this.trackDataStatus}
onTrackVisibilityChange={this.onTrackVisibilityChange}
getLollipopColor={getColorForProteinImpactType}
filterResetPanel={
!this.props.store.dataStore.showingAllData && this.filterResetPanel ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you just pass filterResetPanel whether it's defined or not? just kind confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem here is, for some reason, we initially defined filterResetPanel as JSX.Element | null instead of JSX.Element | undefined. When I just pass it, TS complains that null cannot be assigned to undefined.

For now just updated the check to this.filterResetPanel !== null, but we should probably revisit those function definitions (both here and in react-mutation-mapper)

export const PROTEIN_IMPACT_TYPE_FILTER_ID = "_cBioPortalProteinImpactTypeFilter_";
export const MUTATION_STATUS_FILTER_ID = "_cBioPortalMutationStatusFilter_";

export function findMutationTypeFilter(dataFilters: DataFilter[])
Copy link
Collaborator

Choose a reason for hiding this comment

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

function name sees strange because code finds a particular type of filter, right? shouldn't function name suggest that?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to findProteinImpactTypeFilter

@onursumer onursumer force-pushed the react-mutation-mapper-0.3.0 branch 2 times, most recently from 5a16964 to 6edcc41 Compare November 19, 2019 02:05
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 19, 2019 02:05 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 19, 2019 17:04 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 19, 2019 18:06 Inactive
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 19, 2019 20:31 Inactive
Signed-off-by: Onur Sumer <s.onur.sumer@gmail.com>

replaced ProteinImpactTypePanel with ProteinImpactTypeBadgeSelector

Signed-off-by: Onur Sumer <s.onur.sumer@gmail.com>

enabled filtering for MutationRateSummary

Signed-off-by: Onur Sumer <s.onur.sumer@gmail.com>

fixed mutation type filter counts

Signed-off-by: Onur Sumer <s.onur.sumer@gmail.com>
Signed-off-by: Onur Sumer <s.onur.sumer@gmail.com>
@jjgao jjgao temporarily deployed to cbioportal-f-react-muta-ju2brr November 19, 2019 21:49 Inactive
@alisman alisman merged commit 4789052 into cBioPortal:master Nov 19, 2019
@onursumer onursumer deleted the react-mutation-mapper-0.3.0 branch November 20, 2019 17:02
@inodb inodb added the feature label Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants