Skip to content

Fixed expand/collapse shortcut to not conflict with browser shortcuts - #33797

Merged
bors merged 2 commits into
rust-lang:masterfrom
alex-ozdemir:rustdoc
May 25, 2016
Merged

Fixed expand/collapse shortcut to not conflict with browser shortcuts#33797
bors merged 2 commits into
rust-lang:masterfrom
alex-ozdemir:rustdoc

Conversation

@alex-ozdemir

@alex-ozdemiralex-ozdemir commented May 22, 2016

Copy link
Copy Markdown
Contributor

Allows both T and t.

It had been [Shift]+[+] before.

In response to #33791.

cc @Manishearth
r? @GuillaumeGomez

@alex-ozdemir
Changed toggle all sections key to T
01dc9f0
Allows both `T` and `t`.
It had been [Shift]+[+] before.
@rust-highfive

rust-highfive commented May 22, 2016

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (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. Due to 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.

GuillaumeGomez
GuillaumeGomez reviewed May 22, 2016
View reviewed changes
Comment threadsrc/librustdoc/html/static/main.js Outdated
break;

case "+":
case "t":

@GuillaumeGomezGuillaumeGomezMay 22, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you're writing into the search field, does it expand all sections or not?

@GuillaumeGomez

GuillaumeGomez commented May 22, 2016

Copy link
Copy Markdown
Member

Also, I open a new tab with "CTRL+T". Does it expand all sections?

@Manishearth

Manishearth commented May 22, 2016

Copy link
Copy Markdown
Member

Can't be t, since its already a well known shortcut.

How about e or slash?

@alex-ozdemir

alex-ozdemir commented May 23, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Copying one of @GuillaumeGomez's comments here:

Adding another key to the "+" I guess? Something like "SHIFT+'+'" maybe?

One thing to note - it currently is [Shift] + [+=], rather than just [+=]. If we like that we can leave it.

Otherwise any of "e", [?/], or [|] seem fine to me, although [Shift] + [?/] brings up the help screen.

@ollie27

ollie27 commented May 23, 2016

Copy link
Copy Markdown
Contributor

[Ctrl]+[T] isn't a problem because of these lines. Neither is [Ctrl]+[+] for that matter.

The shortcut doesn't work properly at the moment anyway because it's missing ev.preventDefault();.

@alex-ozdemir
Fixed shortcut handling. Reverted to [Shift]+[+=]
57ab0ed
Realized browsers use [Ctrl]+[+=] for zoom, so using [Shift]+[+=] for
collapse/expand was not necessarily a conflict.
Also a bugfix.
@alex-ozdemir

alex-ozdemir commented May 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Thanks! I was only testing on Chrome, so I didn't realize the ev.preventDefault() was necessary.

@Manishearth

Manishearth commented May 23, 2016

Copy link
Copy Markdown
Member

Ah, okay. Could you upload a copy of this running somewhere?

@GuillaumeGomez

GuillaumeGomez commented May 23, 2016

Copy link
Copy Markdown
Member

Ah, okay. Could you upload a copy of this running somewhere?

I'd like to test it as well.

@alex-ozdemir

alex-ozdemir commented May 23, 2016

Copy link
Copy Markdown
ContributorAuthor

Good idea.

The documentation for std built with the new version of rustdoc is here.

@Manishearth

Manishearth commented May 23, 2016

Copy link
Copy Markdown
Member

looks good to me

@bors delegate=GuillaumeGomez

@bors

bors commented May 23, 2016

Copy link
Copy Markdown
Collaborator

✌️ @GuillaumeGomez can now approve this pull request

@GuillaumeGomez

GuillaumeGomez commented May 23, 2016

Copy link
Copy Markdown
Member

Good for me as well. Thanks @alex-ozdemir!

@bors: r+ rollup

@bors

bors commented May 23, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 57ab0ed has been approved by GuillaumeGomez

@alex-ozdemiralex-ozdemir changed the title Changed toggle all sections key to TFixed expand/collapse shortcut to not conflict with browser shortcutsMay 23, 2016
@GuillaumeGomezGuillaumeGomez mentioned this pull request May 24, 2016
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 24, 2016
@GuillaumeGomez
Rollup merge ofrust-lang#33797- alex-ozdemir:rustdoc, r=GuillaumeGomez
1e4a77b
Changed toggle all sections key to `T`
Allows both `T` and `t`.
It had been [Shift]+[+] before.
In response to rust-lang#33791.
cc @Manishearth
r? @GuillaumeGomez
bors added a commit that referenced this pull request May 24, 2016
@bors
Auto merge of#33833- GuillaumeGomez:rollup, r=GuillaumeGomez
8393d99
Rollup of 7 pull requests
- Successful merges: #33692, #33759, #33779, #33781, #33797, #33810, #33832
- Failed merges:
@bors
bors merged commit 57ab0ed into rust-lang:masterMay 25, 2016
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@GuillaumeGomezGuillaumeGomez

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@alex-ozdemir@rust-highfive@GuillaumeGomez@Manishearth@ollie27@bors