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

Change file and links name to resource displayName #5061

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hweej
Copy link
Contributor

@hweej hweej commented Dec 11, 2024

Adds a condition to the link text where if all resources are of the same type, then rename the Files & Links tab to the resourceDefinition displayName

@hweej hweej requested review from alisman and inodb December 11, 2024 20:52
Copy link

netlify bot commented Dec 11, 2024

Deploy Preview for cbioportalfrontend ready!

Name Link
🔨 Latest commit ae9281e
🔍 Latest deploy log https://app.netlify.com/sites/cbioportalfrontend/deploys/6759fb7e61ec8a00086809cc
😎 Deploy Preview https://deploy-preview-5061.cancerrevue.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hweej hweej self-assigned this Dec 11, 2024
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much! Tested SPECTRUM and PCAWG studies

Comment on lines +724 to +730
linkText={
this.store.resourceDefinitions
.result?.length == 1
? this.store.resourceDefinitions
.result[0].displayName
: RESOURCES_TAB_NAME
}
Copy link
Member

@inodb inodb Dec 11, 2024

Choose a reason for hiding this comment

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

how does this work when there is only a resource definition for a study resource (e.g. rather than a sample or patient resource)? I guess, technically, it might show up as two tabs with the same name? Prolly ok to not handle this case but just curious

Copy link
Collaborator

Choose a reason for hiding this comment

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

@inodb @hweej the fact that the content of this is this.openResource suggests it can contain many things no? So to just choose the first resourceDefinition doesn't seem right.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah i see. it's if the length is one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hweej i think this will have behavior where tab will have text RESOURCE_TAB_NAME while the resource definitions are loading and then the name will switch. We should probably just the tab until it finishes loading. see hide propery on other tabs in this collection

@inodb inodb changed the title Change file and links name to resource displayName. Change file and links name to resource displayName Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants