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

Fix: Small fix for location hash/fragment #2464

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

BToersche
Copy link
Contributor

@BToersche BToersche commented Mar 24, 2024

The current location hash implementation will replace all # chars in the fragment, which shouldn't be the case.
While, according to spec, # is not allowed in the fragment portion of the URL, browsers tend to be lenient towards enforcing this. Opening a URL in your browser (be it Chrome or Firefox) with # in the fragment will get accepted and returned in your location hash.

Consider https://github.com/leptos-rs/leptos/#test#bar. Your location hash will return (in Javascript) #test#bar.
The current implementation in Leptos will return testbar while the new implementation will return test#bar, which fits the docs better of the function:

/// Current [`window.location.hash`](https://developer.mozilla.org/en-US/docs/Web/API/Window/location)
/// without the beginning #.

A small side-note: I've contemplated simply always removing the first char of the fragment (since this should always be # anyways). Like so: if s.is_empty() { s } else { s[1..].to_string() }. I've decided against it, but if you (the reviewer) prefers this implementation, please let me know and I'll update it.

@benwis
Copy link
Contributor

benwis commented Mar 26, 2024

Hi! Your solution seems fine, if a bit verbose.

@benwis
Copy link
Contributor

benwis commented Mar 27, 2024

Thanks for fixing this!

@benwis benwis merged commit 73b8c78 into leptos-rs:main Mar 27, 2024
35 of 62 checks passed
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.

2 participants