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.

Sign up for GitHub

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

Jump to bottom

Add HashProtocol #50

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Add HashProtocol #50

merged 1 commit into from
Jan 9, 2018

Conversation

taion
Copy link
Contributor

@taion taion commented Dec 29, 2017 •
edited
Loading

Closes #21

taion
taion commented Dec 29, 2017
View reviewed changes
src/HashProtocol.js
}

init() {
// TODO: Do we still need to work around the old Firefox bug here?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a well-known Firefox bug, but I don't see it any more. Is it worth keeping the old workaround?

@jquense
Copy link
Member

jquense commented Dec 29, 2017 •
edited
Loading

Is it "update my OSS projects" week? :P

@taion
Copy link
Contributor Author

taion commented Dec 29, 2017

It's itch-scratching week!

@taion taion requested a review from jquense January 7, 2018 16:41
jquense
jquense approved these changes Jan 7, 2018
View reviewed changes
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

LGTM, do you not need to store the key in the url, like history did, or are we just saying no to that

@taion
Copy link
Contributor Author

taion commented Jan 7, 2018

I’m not going to bother for now. It was a weird feature anyway, and for most use cases, using the full path as key is sufficient.

@jquense
Copy link
Member

jquense commented Jan 7, 2018

fine by me, the use cases for this Protocol are increasingly minimal anyway

@taion
Copy link
Contributor Author

taion commented Jan 7, 2018

It’s really just for TodoMVC ;)

@taion taion merged commit 017d2c6 into master Jan 9, 2018
@taion taion deleted the HashProtocol branch January 9, 2018 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jquense jquense jquense approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@taion @jquense