-
Notifications
You must be signed in to change notification settings - Fork 270
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 Software acknowledgment page #3058
Conversation
@@ -44,6 +44,7 @@ const ServerConfigDefaults: Partial<IServerConfig> = { | |||
skin_authorization_message: | |||
'Access to this portal is only available to authorized users.', | |||
skin_documentation_about: 'About-Us.md', | |||
skin_documentation_software: 'Software-Acknowledgments.md', |
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.
Nice US spelling of acknowledgment
FYI - If we want to allow people to update this config using portal.properties it will also need to be added to config_service.jsp in the backend. We already allow people to override the footer in its entirety, so seems overkill to add it there but just so you know
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 to me! Thanks for doing this!
One minor thing is that I personally prefer imperative mood in the subject line:
https://chris.beams.io/posts/git-commit/#seven-rules
I find it makes the title more concise and therefore easier to read. Same with PR titles
Opinions about this vary tho
@inodb: Good tip on imperative. So, instead of "Added new route for software page" --> "Add new route for software page"? Write back! (I think that's imperative :-) |
Lol yes 🙂! |
Fixes cBioPortal/cbioportal#5667
skin_documentation_software
./software
router./software
within the footer.Any screenshots or GIFs?
Notify reviewers
@alisman @inodb or @jjgao