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

Adding pushState based location implementation #945

Merged
merged 6 commits into from Jun 6, 2012

Conversation

Projects
None yet
6 participants
@digitaltoad
Contributor

digitaltoad commented Jun 4, 2012

No description provided.

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jun 5, 2012

Member

Thank's

  • there is no test for popstate change
  • you should probably split location file loaction (factory) / hash_loacation / history_location

Finaly, in my opinion it should be called HistoryLocation and not HTML5Location...

Member

tchak commented Jun 5, 2012

Thank's

  • there is no test for popstate change
  • you should probably split location file loaction (factory) / hash_loacation / history_location

Finaly, in my opinion it should be called HistoryLocation and not HTML5Location...

@digitaltoad

This comment has been minimized.

Show comment
Hide comment
@digitaltoad

digitaltoad Jun 5, 2012

Contributor

HistoryLocation sounds good to me. Should the registered keyword also be history? Also, do you mean that I should split the implementations off from Location into separate files?

Contributor

digitaltoad commented Jun 5, 2012

HistoryLocation sounds good to me. Should the registered keyword also be history? Also, do you mean that I should split the implementations off from Location into separate files?

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jun 5, 2012

Member

yes and yes :)

Member

tchak commented Jun 5, 2012

yes and yes :)

@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Jun 5, 2012

Contributor

👍

Contributor

frodsan commented Jun 5, 2012

👍

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Jun 6, 2012

Member

This seems useful. Would be good for someone who has worked more with the new routing stuff to review though. @wycats, @tomdale?

Member

wagenet commented Jun 6, 2012

This seems useful. Would be good for someone who has worked more with the new routing stuff to review though. @wycats, @tomdale?

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jun 6, 2012

Member

@digitaltoad lastSetURL is not part of the public API, and shouldn't be necessary in the pushstate impl, I think?

Member

wycats commented Jun 6, 2012

@digitaltoad lastSetURL is not part of the public API, and shouldn't be necessary in the pushstate impl, I think?

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged 78ff347 into 9676856).

travisbot commented Jun 6, 2012

This pull request passes (merged 78ff347 into 9676856).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged c34a042 into 9676856).

travisbot commented Jun 6, 2012

This pull request passes (merged c34a042 into 9676856).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged 5e2eef5 into 9676856).

travisbot commented Jun 6, 2012

This pull request passes (merged 5e2eef5 into 9676856).

wycats added a commit that referenced this pull request Jun 6, 2012

Merge pull request #945 from digitaltoad/feature/html5_location
Adding pushState based location implementation

@wycats wycats merged commit 4afbe7b into emberjs:master Jun 6, 2012

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