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

expose component renderers for legacy project #89

Merged
merged 1 commit into from
Dec 1, 2016

Conversation

alisman
Copy link
Collaborator

@alisman alisman commented Nov 30, 2016

What? Why?

As we build components in the new project, we need a good way for code in the legacy project to mount the components in various places on the page. We tried doing this from within the new project, but I realized this isn't sufficient. For example, if we need to mount a component inside a jqueryUI tab in the legacy site. We don't want to do that on page load, but when the tab is shown. That's a key optimization and it would be a step backward to do otherwise. So the legacy project needs to be able to control the mounting of the new components in some cases. Be exposing render functions (which take the target node as arg) on the window object we can grant control to the legacy code.

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:

}
}
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this file prefixed with "MSK?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. No great reason.


interface IPatientViewPageProps {
store?: RootState;
}

@MSKPageDecorator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be a little too fancy, but what this decorator does is disable the page components render function when we detect that we are in the legacy site context. In this context, it's then at the discretion of the legacy code to invoke the renderers which we expose on the window object.
What I like about this solution is that we still have page specific bundles which include all the components for a given page. In dev context, we can render these as per nomal in the page component. But in legacy project, that may not be appropriate because we want to render the components on various places in the page and at various times. This solution allows this freedom.

@@ -0,0 +1,13 @@
import * as React from 'react';

export default function (component: typeof React.Component): any {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adufilie , I believe return type of this should be React.Component but when I put that in, I get a weird error that I don't understand. Generic type 'Component<P, S>' requires 2 type argument(s).

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't matter much. The current decorator type restrictions are a little inconvenient. It wants the return type to be exactly the same as the parameter type.

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.

This is useful!

  • Fix spelling
  • MSK prefix

@@ -43,7 +44,7 @@ export default class ClinicalInformationContainer extends React.Component<IClini
return <div>{ this.buildTabs() }</div>;

case 'error':
return <div>There was an error.</div>;
return <div>There was an loading error. Please try refreshing your browser.</div>;
Copy link
Member

Choose a reason for hiding this comment

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

*a loading

@alisman alisman merged commit d033949 into cBioPortal:master Dec 1, 2016
@alisman alisman deleted the renderers branch December 1, 2016 20:18
onursumer added a commit to onursumer/cbioportal-frontend that referenced this pull request Dec 18, 2019
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 11, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 16, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 18, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 19, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 20, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 23, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 24, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 25, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 26, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request May 27, 2022
…vements

improvements to custom track options
alisman pushed a commit to alisman/cbioportal-frontend that referenced this pull request Jun 2, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022
…vements

improvements to custom track options
onursumer pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 6, 2022
…vements

improvements to custom track options
alisman pushed a commit to onursumer/cbioportal-frontend that referenced this pull request Jun 8, 2022
…vements

improvements to custom track options
nr23730 added a commit to buschlab/cbioportal-frontend that referenced this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants