Uh oh!
There was an error while loading. Please reload this page.
fixes #28696 - #28725
Conversation
rust-highfive
commented
Sep 29, 2015
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
marti1125
commented
Sep 29, 2015
@steveklabnik is fixed this issue |
steveklabnik
commented
Sep 29, 2015
I reviewed this locally and it works well, but I'd like someone else to sign off on it too. |
There was a problem hiding this comment.
Either use replaceState, or ensure that we only push at most 1 instance of this. Otherwise we just mess up the browser’s history stack.
There was a problem hiding this comment.
thanks for the advice @nagisa I am going to change that
There was a problem hiding this comment.
Ah, also, search event is webkit (safari, epiphany) specific. It is not standard, nor it is supported widely.
There was a problem hiding this comment.
I added search for webkit for ("X") in chrome... or maybe remove X in chrome... what do you think?
There was a problem hiding this comment.
Just use a standard event (input should do), like in this PR.
steveklabnik
commented
Sep 30, 2015
@nagisa is happy with this, I'm happy with this. r=me after a squash. |
alexcrichton
commented
Sep 30, 2015
Yes thanks @marti1125! This also looks good to me as well. Can you also expand the commit messages to have a more detailed description of what was changed, what was fixed, and how it works? It's helpful when browsing the history of the repo to have a little more context than "update main.js" and "fixes #xxxx" :) |
marti1125
commented
Sep 30, 2015
@alexcrichton I think is done =D |
alexcrichton
commented
Sep 30, 2015
Thanks! Could you also squash all the commits together and perhaps merge some of the messages? You can find some good examples of how to write nice commit messages here. |
marti1125
commented
Sep 30, 2015
@alexcrichton I am going to create a new clean PR with solution |
#28696