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.

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

restore the page title after escaping out of a search #53405

Merged
merged 1 commit into from
Aug 21, 2018

Conversation

oconnor663
Copy link
Contributor

Currently if I start a search in the docs, but then hit ESC, the "Results for..." title is still there in my browser tab. This is a simple attempt to fix that. I see that there's a separate var previousTitle = document.title thing happening in startSearch(), but as far as I can tell that's only related to the back stack? I'd also appreciate feedback on the right place to declare the titleBeforeSearch variable.

Testing-wise, I've confirmed by hand that the tab title restores correctly after building with ./x.py doc --stage 1 src/libstd, but nothing more involved than that. What else should I test?

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2018
@GuillaumeGomez
Copy link
Member

I have to admit that I never paid attention to it... From what I can see, the only cases for title change are:

  1. you click on an item and then change page (or go to a link to the current one)
  2. you press ESC

Actually, does it change the document title correctly when you pick an item from the current page?

@oconnor663
Copy link
Contributor Author

oconnor663 commented Aug 16, 2018

Picking an item from the current page works fine when I tried it just now. But I noticed a different bug:

  1. Type "f" in the search box.
  2. Title changes to "Results for f".
  3. Type "o" in the search box.
  4. Title changes to "Results for fo".
  5. Hit ESC.

Bug: Title changes back to "Results for f" :(

If search is the only thing other than a pageload that changes the title, like you said, maybe I can unconditionally stash the title once at the beginning, and only ever restore that particular string? I'll try that approach and rebase.

@oconnor663
Copy link
Contributor Author

This version is only two lines, but maybe you can help me make sure I'm not assuming too much about how the whole page is architected. I'm totally unfamiliar with this code in general :-D

@GuillaumeGomez
Copy link
Member

No problem, you did great. Two solutions in this situation:

  1. check if the title starts with "Results for " before setting it into the previousTitle variable
  2. set back the previousTitle before every time you want to change it to "Results for " title

@oconnor663
Copy link
Contributor Author

Did you see the approach in 2075509. Let me know what you think of that one.

@GuillaumeGomez
Copy link
Member

Oh that's good too. You just set it once you arrive on the page so I assume it's all good. If it's ok for you then I'll r+ it.

@oconnor663
Copy link
Contributor Author

Yeah I think I'm OK with it as is, as long as we don't expect to do any fancy partial page loading shenanigans in the near future that'll make this incorrect?

@GuillaumeGomez
Copy link
Member

We'll think about it at this moment but nothing like this is scheduled for the moment. Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 18, 2018

📌 Commit 2075509 has been approved by GuillaumeGomez

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 19, 2018
…omez

restore the page title after escaping out of a search

Currently if I start a search in the docs, but then hit ESC, the "Results for..." title is still there in my browser tab. This is a simple attempt to fix that. I see that there's a separate `var previousTitle = document.title` thing happening in `startSearch()`, but as far as I can tell that's only related to the back stack? I'd also appreciate feedback on the right place to declare the `titleBeforeSearch` variable.

Testing-wise, I've confirmed by hand that the tab title restores correctly after building with `./x.py doc --stage 1 src/libstd`, but nothing more involved than that. What else should I test?
@oconnor663
Copy link
Contributor Author

Thanks for reviewing!

kennytm added a commit to kennytm/rust that referenced this pull request Aug 21, 2018
…omez

restore the page title after escaping out of a search

Currently if I start a search in the docs, but then hit ESC, the "Results for..." title is still there in my browser tab. This is a simple attempt to fix that. I see that there's a separate `var previousTitle = document.title` thing happening in `startSearch()`, but as far as I can tell that's only related to the back stack? I'd also appreciate feedback on the right place to declare the `titleBeforeSearch` variable.

Testing-wise, I've confirmed by hand that the tab title restores correctly after building with `./x.py doc --stage 1 src/libstd`, but nothing more involved than that. What else should I test?
bors added a commit that referenced this pull request Aug 21, 2018
Rollup of 17 pull requests

Successful merges:

 - #53030 (Updated RELEASES.md for 1.29.0)
 - #53104 (expand the documentation on the `Unpin` trait)
 - #53213 (Stabilize IP associated constants)
 - #53296 (When closure with no arguments was expected, suggest wrapping)
 - #53329 (Replace usages of ptr::offset with ptr::{add,sub}.)
 - #53363 (add individual docs to `core::num::NonZero*`)
 - #53370 (Stabilize macro_vis_matcher)
 - #53393 (Mark libserialize functions as inline)
 - #53405 (restore the page title after escaping out of a search)
 - #53452 (Change target triple used to check for lldb in build-manifest)
 - #53462 (Document Box::into_raw returns non-null ptr)
 - #53465 (Remove LinkMeta struct)
 - #53492 (update lld submodule to include RISCV patch)
 - #53496 (Fix typos found by codespell.)
 - #53521 (syntax: Optimize some literal parsing)
 - #53540 (Moved issue-53157.rs into src/test/ui/consts/const-eval/)
 - #53551 (Avoid some Place clones.)

Failed merges:

r? @ghost
@bors bors merged commit 2075509 into rust-lang:master Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants