-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Migrate signup page to angular router. #13582
Migrate signup page to angular router. #13582
Conversation
Hi @ashutoshc8101, can you complete the following:
|
Hi, @ashutoshc8101, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @ashutoshc8101 to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks! |
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!
Unassigning @vojtechjelinek since they have already approved the PR. |
Unassigning @nithusha21 since they have already approved the PR. |
Hi @ashutoshc8101, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
Hi @ashutoshc8101. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
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 for the codeowner file main.py
left or comment, PTAL!
URLS.append( | ||
get_redirect_route( | ||
r'/%s' % page['ROUTE'], oppia_root.OppiaRootPage)) | ||
if not 'MANUALLY_REGISTERED_WITH_BACKEND' in page: |
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.
Can you explain what's MANUALLY_REGISTERED_WITH_BACKEND
and why it is introduced? :)
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 URLs contain URL fragments and they have different syntax for angular router and backend.
eg.
Url for backend: /learn/<classroom_url_fragment>
url for angular router: /learn/:classroom_url_fragment
So, I have used the second case in constant.ts (so, it works directly with the angular router).
And manually registered these routes with fragments in the backend.
That's why I am excluding these routes in this loop.
This explanation is for other pages with URL fragments in PR #13533.
For, /signup route, it does not use the common Handler OppiaRootPage
unlike others. It uses SignupPageHandler as it performs additional checks too.
@DubeySandeep PTAL |
Unassigning @darksun27 since they have already approved the PR. |
Hi @ashutoshc8101, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
Unassigning @DubeySandeep since they have already approved the PR. |
Overview
Essential Checklist
Proof that changes are correct
signup-page-2021-08-05_14.49.35.mp4
PR Pointers