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

web: Move root routes into a shared constant #26177

Merged
merged 4 commits into from Oct 26, 2021

Conversation

xg-wang
Copy link
Contributor

@xg-wang xg-wang commented Oct 17, 2021

Summary

This PR moves root routes into a shared constant. Sub routes are not (yet) moved to this top constant.

fixes #20693

Testing

Passed these:

yarn test
yarn all:eslint
yarn prettier-check
yarn all:stylelint
yarn graphql-lint

Note yarn integration-test fails, which also fails on main branch locally.

@cla-bot
Copy link

cla-bot bot commented Oct 17, 2021

We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please sign CLA to get yourself added.

For reviewers: to request a Buildkite build for a pull request from a fork, check out the branch and use sg ci build after reviewing the code.

@xg-wang
Copy link
Contributor Author

xg-wang commented Oct 17, 2021

@natectang CLA signed

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Beautiful! Thank you for updating logic in client/web/src/Layout.tsx to use the PageRoutes enum.

Could you update one thing before merging: change enums key casing to CamelCase from UPPER_CASE? We don't have an ESLint rule setup for that yet, but that's the approach used for most of the new functionality in the codebase.

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Also, it seems that pulling main into this branch is required to resolve the merge conflict.

@valerybugakov
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Oct 22, 2021
@cla-bot
Copy link

cla-bot bot commented Oct 22, 2021

The GitHub CLA Bot is rechecking to see that you have signed the CLA

client/web/src/Layout.tsx Outdated Show resolved Hide resolved
@xg-wang
Copy link
Contributor Author

xg-wang commented Oct 22, 2021

@valerybugakov thanks for reviewing, I fixed the conflicts and changed the format

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

change enums key casing to CamelCase from UPPER_CASE

It seems I didn't describe the change above adequately. I should've added an example. Added it to one of the lines changed now.

client/web/src/Layout.tsx Outdated Show resolved Hide resolved
@xg-wang xg-wang marked this pull request as draft October 25, 2021 15:37
Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

Excellent, thank you! It's a good start in making our routing more robust.

@valerybugakov valerybugakov merged commit 2d0a854 into sourcegraph:main Oct 26, 2021
@xg-wang xg-wang deleted the xgwang/routes branch October 26, 2021 13:46
@ltagliaferri
Copy link

Hi @xg-wang, Sourcegraph would love to send you a Hacktoberfest treat, please email us at dev-ed@sourcegraph.com and reference this PR so we can set you up 🎃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move root routes into a shared constant
3 participants