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

Rewrite angular service - customURL #1042

Merged

Conversation

jiji14
Copy link
Contributor

@jiji14 jiji14 commented Sep 28, 2023

Rewrite customURL.js angular service

Related Issue : e-mission/e-mission-docs#987

While rewriting the customURL.js, I noticed that CustomURL.onLaunch is only called once in handleOpenURL in join-ctrl.js. I removed the angular event handlers $broadcast and $on, and instead, I directly called the onLaunchCustomURL within handleOpenURL. As a result, I no longer needed the angular event handler in customURL.ts.

I added test cases with valid url and invalid url.

@jiji14 jiji14 marked this pull request as draft September 28, 2023 18:46
@jiji14 jiji14 changed the title Rewrite angular service Rewrite angular service - customURL Sep 28, 2023
@jiji14 jiji14 marked this pull request as ready for review September 29, 2023 21:29
@jiji14 jiji14 marked this pull request as draft September 29, 2023 23:06
@jiji14 jiji14 changed the base branch from master to service_rewrite_2023 September 29, 2023 23:32
@jiji14 jiji14 marked this pull request as ready for review September 30, 2023 06:15
@JGreenlee
Copy link
Collaborator

JGreenlee commented Oct 3, 2023

Please add this to "Ready for review" on the projects board: https://github.com/orgs/e-mission/projects/1

Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

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

Great work! LGTM!

I do think these changes are going to conflict with my changes in #1040 because this PR modifies join-ctrl.js, and join-ctrl.js no longer exists in my PR.

I think it's better to first get this PR merged into service_rewrite_2023 and then I can fix conflicts on my PR. In the future I will ensure that we use a better branch management strategy.

@JGreenlee
Copy link
Collaborator

@jiji14 Since this PR has been open for 2 weeks and didn't get merged quickly, there are some fairly complex merge conflicts here.
Would you like me to address them?

@jiji14
Copy link
Contributor Author

jiji14 commented Oct 12, 2023

@jiji14 Since this PR has been open for 2 weeks and didn't get merged quickly, there are some fairly complex merge conflicts here.
Would you like me to address them?

Yes, please 🆘

@JGreenlee
Copy link
Collaborator

I addressed the merge conflicts. LGTM

(as a side note, I am not sure that we actually need customURL moving forward as we no longer use links to trigger the onboarding flow. However, I think it's still useful to keep this around)

@jiji14
Copy link
Contributor Author

jiji14 commented Oct 13, 2023

I addressed the merge conflicts. LGTM

(as a side note, I am not sure that we actually need customURL moving forward as we no longer use links to trigger the onboarding flow. However, I think it's still useful to keep this around)

Looks good to me too and I agree to keep it! 👍

@jiji14 jiji14 requested a review from shankari October 19, 2023 20:42
@shankari
Copy link
Contributor

I am merging for now, but we should remove it after ~ 1 year if we are not using it.

@shankari shankari merged commit 47fa869 into e-mission:service_rewrite_2023 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants