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

Already on GitHub? Sign in to your account

added possibility to use urls with query string #8

Merged
merged 2 commits into from Mar 5, 2012

Conversation

Projects
None yet
2 participants
Contributor

ausi commented Mar 5, 2012

No description provided.

@cpojer cpojer and 1 other commented on an outdated diff Mar 5, 2012

Source/History.js
@@ -42,24 +48,28 @@ this.History = new new Class({
this.timer = this.check.periodical(200, this);
},
+ getUrlPart: getUrlPart,
@cpojer

cpojer Mar 5, 2012

Owner

Why does this need to be public?

@ausi

ausi Mar 5, 2012

Contributor

because it is used in History.handleInitialState.js, is there a better way to do this?

@cpojer

cpojer Mar 5, 2012

Owner

Oh wow, totally missed that. I guess it is fine then.

@cpojer cpojer and 1 other commented on an outdated diff Mar 5, 2012

Source/History.js
@@ -20,7 +20,13 @@ provides: History
var events = Element.NativeEvents,
location = window.location,
- base = location.pathname,
+ getUrlPart = function(url){
+ if(url.substr(0,8) == 'https://' || url.substr(0,7) == 'http://'){
+ url = '/'+url.split('/').slice(3).join('/');
+ }
@cpojer

cpojer Mar 5, 2012

Owner

would you mind updating the code style here to match the project's code style?

Something like this:

if (url.match(/https?:///) url = '/' + url.split('/').slice(3).join('/');

Also, why not just name the method cleanURL(), getURL() or something simple like that?

@ausi

ausi Mar 5, 2012

Contributor

done

Owner

cpojer commented Mar 5, 2012

Awesome! I've been meaning to do this but never got around to it. Would you mind addressing my inline comments?

@ausi ausi renamed getUrlPart to cleanURL
updated code style to match project's code style
0e085a7
Contributor

ausi commented Mar 5, 2012

I've changed the code according to your inline comments.
Thanks for this great MooTools Class!

@cpojer cpojer added a commit that referenced this pull request Mar 5, 2012

@cpojer cpojer Merge pull request #8 from ausi/feature-urls-with-query
added possibility to use urls with query string
48417cd

@cpojer cpojer merged commit 48417cd into cpojer:master Mar 5, 2012

Owner

cpojer commented Mar 5, 2012

awesome! Thank you.

Owner

cpojer commented Mar 5, 2012

Is there a reason you are removing the #hash part of the URL? I'd like to keep that.

Contributor

ausi commented Mar 6, 2012

I thought not removing the #hash would cause problems in browsers not supporting push state, but i didn't tested it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment