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

getQuery with hash before #227

Merged
merged 3 commits into from Mar 3, 2016
Merged

getQuery with hash before #227

merged 3 commits into from Mar 3, 2016

Conversation

diegoleme
Copy link
Contributor

referent #226

@@ -4,7 +4,7 @@ define(function () {
* Gets full query as string with all special chars decoded.
*/
function getQuery(url) {
url = url.replace(/#.*/, ''); //removes hash (to avoid getting hash query)
url = url.replace(/#.*\?/, '?'); //removes hash (to avoid getting hash query)
Copy link
Member

Choose a reason for hiding this comment

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

instead of replacing the RegExp you could simply delete this line if you want to allow getting the ?.+ after the #.

this was there because in some cases you might have URLs like: http://example.com/foo?bar=true#lorem?ipsum=dolor, so what should be the result of getQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The correct is query to come before the hash, but, on a SPA, a URL with query string before it's weird:

https://example.com/#/users/
https://example.com/#/users/?page=2

The solution to my case can be:
https://example.com/#/users/page-2/

But I wonder if the case should be valid only "?bar=true" or "?bar=true&ipsum=dolor"
http://example.com/foo?bar=true#lorem?ipsum=dolor

Copy link
Member

Choose a reason for hiding this comment

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

I think it should grab the first query string and ignore the second (if string contains 2). so let's just delete the line 7 and add more tests to show behavior when we have multiple queries.

@millermedeiros millermedeiros added this to the v0.12.0 milestone Apr 22, 2015
@roboshoes roboshoes merged commit 5eb0636 into mout:master Mar 3, 2016
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.

None yet

3 participants