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

Add somatic/germline gene page #1181

Merged
merged 21 commits into from
Oct 25, 2024
Merged

Add somatic/germline gene page #1181

merged 21 commits into from
Oct 25, 2024

Conversation

zhx828
Copy link
Member

@zhx828 zhx828 commented Oct 23, 2024

No description provided.

@zhx828 zhx828 requested a review from jfkonecn October 23, 2024 17:25
routing={this.props.routing}
windowStore={this.props.windowStore}
>
<AppRouts
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a typo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, will fix

Comment on lines +32 to +36
inGenePage
? ''
: props.windowStore.isXLscreen
? 'container'
: 'container-fluid'
Copy link
Contributor

Choose a reason for hiding this comment

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

Would CSS media queries would this be simpler than using Javascript?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. Didn't look into, inherited from the original.

Copy link
Member Author

Choose a reason for hiding this comment

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

For public website, we haven't really used the @media to manage the layout. I would stick to the classname for now.

content: (
<div>
<OncoKBLevelIcon
size={'s2'}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this all over the code base is there a reason why you don't do "s2" instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

you meant why it's not a default?

Copy link
Contributor

Choose a reason for hiding this comment

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

size={'s2'} vs size="s2"


@inject('appStore', 'windowStore', 'routing')
@observer
export default class SomaticGermlineGenePage extends React.Component<
Copy link
Contributor

Choose a reason for hiding this comment

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

why not a functional component?

Copy link
Member Author

Choose a reason for hiding this comment

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

We have a store in this class. And we manage some observables states here too.

@jfkonecn jfkonecn self-requested a review October 24, 2024 17:51
@zhx828 zhx828 merged commit 067dcd3 into master Oct 25, 2024
7 of 8 checks passed
@zhx828 zhx828 deleted the feature/germline branch October 25, 2024 02:18
@zhx828 zhx828 added the feature Feature tag for release label Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature tag for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants