-
Notifications
You must be signed in to change notification settings - Fork 43
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
Update order of links on NavBar #1150
Conversation
@rileyhgrant can you please advise @iamsadat on how to get checks to pass? |
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.
Hey @iamsadat
Thanks for looking at this! Here are a few comments we'd like you to address:
- The convention for naming both Pull Requests and Commits in this Repo is to use present tense, sentence case, and to be descriptive. i.e. the title should be "Update order of links on NavBar" and similarly the commit message should be changed to something like "Update order of links on the NavBar"
- We use Jest for testing on the frontend. For many of these simpler components the tests are simple snapshot tests. On your local machine, this should be as simple as running
yarn jest --no-cache
from the root directory, followed by ayarn jest --no-cache -u <YOUR-TEST-FILE>
, in this case something like:yarn jest --no-cache -u NavBar
. The-u
flag specifies that Jest should update the snapshot generated by the test. - As mentioned in the issue, this will have to be merged in sync with a PR on the gnomAD Blog, since there are two seperate applications. Since I'll be merging this, I can make the other PR.
Thanks again for taking a look at this, this also helps us get our docs in better order (we can do a better job of specifying our conventions, as well as documenting what tests to run locally). Let me know if anything is unclear.
Riley
Hey Riley, Thanks for the help and sorry about the mess I've created. But I've used the naming convention as per the repo. The snapshot has been updated too. |
@iamsadat Just wanted to swing by and say thanks for contributing! |
@phildarnowsky-broad |
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.
LGTM, thanks @iamsadat!
I'll merge this PR and squash all the commits into a single one.
Thanks again :)
Fixes #1149