Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

docs: Call super in shouldNavigateTo override #214

Closed
wants to merge 2 commits into from

Conversation

ghiculescu
Copy link
Contributor

Since #163, the base implementation of shouldNavigateTo is useful because it checks for invalid URLs. So if you override this method we should still recommend you call super to get the base implementation.

While here I turned the when into an if per this, but I'm certainly not a Kotlin expert and happy to change it back if you prefer.

Since hotwired#163, the base implementation of `shouldNavigateTo` is useful because it checks for invalid URLs. So if you override this method we should still recommend you call `super` to get the base implementation.

While here I turned the `when` into an `if` per [this](https://kotlinlang.org/docs/coding-conventions.html#if-versus-when), but I'm certainly not a Kotlin expert and happy to change it back if you prefer.
@jayohms
Copy link
Collaborator

jayohms commented Feb 12, 2022

@ghiculescu Thanks for this recommendation. I'm thinking it should probably be easier to register your domain with the library, so by default the library takes care of this logic on its own 🤔. This recommendation actually leaves an open to spoofed urls like:

https://app.hey.com@fake.domain

I need to think about this, but I think we need a new default implementation that allows you to register your app's domain.

@jayohms
Copy link
Collaborator

jayohms commented Feb 23, 2024

Closing this since we have plans to better handle domain/routing logic in a more comprehensive way.

@jayohms jayohms closed this Feb 23, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants