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
Create feedback form for trials #2869
Conversation
747f1cf
to
1a82259
Compare
<span className={styles.statusBackground}>{trial.status}</span> | ||
</a> | ||
<span className={styles.feedback}> | ||
<button className={"btn btn-default btn-sm btn-xs " + styles.feedbackButton} onClick={() => this.showTrialFeedback=true}>Feedback</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a Button component from react-bootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to Button component
<div className={styles.statusButton}> | ||
<a target="_blank" href={"https://www.mskcc.org/cancer-care/clinical-trials/" + trial.protocolNo}><span className={styles.statusBackground}>{trial.status}</span></a> | ||
<div className={styles.statusContainer}> | ||
<a target="_blank" href={"https://www.mskcc.org/cancer-care/clinical-trials/" + trial.protocolNo}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use Button from react-bootstrap, and variant="link"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some css styles cannot apply for the Button component. Also, this is a link not a button. I don't want to mix them together.
@@ -306,6 +327,13 @@ export default class TrialMatchTable extends React.Component<ITrialMatchProps> { | |||
render() { | |||
return ( | |||
<div> | |||
<p style={{marginBottom: '0'}}>Curated genomic and clinical criteria from open clinical trials at Memorial Sloan Kettering. Please <a href="mailto:team@oncokb.org">contact us</a> or submit <a onClick={() => this.showGeneralFeedback=true}>feedback form</a> if you have any questions.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised that changing showGeneralFeedback
on the click event would actually work in mobx observable. @alisman what's the situation that a @autobind
has to be attached?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will work. if you pass it a function directly (not an anonymous callback) then it has to be bound with autobind. Using an anonymous callback is discouraged b/c it causes virtual dom to be differed each time. But in this case, it's totally negligible and not worth changing. So, rule of thumb is, if something is going to be rendered a lot and occurs many times in DOM, then use a bound handler. If not, do the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only few minor comments
6e3b0c5
to
96e4996
Compare
Fix oncokb/oncokb#1816