X Tutup
Skip to content

feat(router): add support for hash-based location#2692

Merged
btford merged 1 commit intoangular:masterfrom
btford:feat-router-hash-location
Jun 23, 2015
Merged

feat(router): add support for hash-based location#2692
btford merged 1 commit intoangular:masterfrom
btford:feat-router-hash-location

Conversation

@btford
Copy link
Copy Markdown
Contributor

@btford btford commented Jun 23, 2015

Closes #2555

@btford btford added feature Label used to distinguish feature request from other issues comp: router labels Jun 23, 2015
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Jun 23, 2015

I'm not sure how to test this (except via an e2e test). Any advice?

@btford btford added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 23, 2015
rkirov
rkirov reviewed Jun 23, 2015
View reviewed changes
modules/angular2/src/router/hash_location_strategy.ts Outdated
Copy link
Copy Markdown
Contributor

@rkirov rkirov Jun 23, 2015

Choose a reason for hiding this comment

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

I notice it is almost identical with Html5LocationStrategy, except one '#'. But it is simple enough to keep as a separate class for now, if things start to get more complication you can explore factoring the commonalities into a base class.

Copy link
Copy Markdown
Contributor Author

@btford btford Jun 23, 2015

Choose a reason for hiding this comment

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

Depending on browser weirdness, I might have to do some shimming WRT history and hashchanges. I think/hope that evergreen browsers do the right thing, but historically that has not been true. So the behavior here in each class might get a bit more complicated. If not, I'll definitely revisit your suggestion 👍

@rkirov
Copy link
Copy Markdown
Contributor

rkirov commented Jun 23, 2015

It is not testable, because our DOM adapter is not injectable. I dont' see an easy wait to fix it, without changing that.

@rkirov rkirov added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 23, 2015
@btford btford force-pushed the feat-router-hash-location branch from fbc8918 to f2b814e Compare June 23, 2015 01:36
@btford btford force-pushed the feat-router-hash-location branch from f2b814e to a67f231 Compare June 23, 2015 01:46
@btford btford merged commit a67f231 into angular:master Jun 23, 2015
@btford
Copy link
Copy Markdown
Contributor Author

btford commented Jun 23, 2015

Landed as a67f231.

@dsebastien
Copy link
Copy Markdown

dsebastien commented Jun 30, 2015

works as expected, thanks!

@btford btford mentioned this pull request Jul 6, 2015
Closed
@btford btford deleted the feat-router-hash-location branch October 23, 2015 13:04
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 7, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@rkirov rkirov

Labels

action: merge The PR is ready for merge by the caretaker cla: yes feature Label used to distinguish feature request from other issues

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(router): support hashchange mode

7 participants

@btford @rkirov @dsebastien @gutenye @tuurbo @rolandjitsu @googlebot
X Tutup