Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign up[Fix for #26016] In the docs js: support keyboard events for non-english keyboard layouts #26675
Conversation
rust-highfive
assigned
alexcrichton
Jun 30, 2015
This comment has been minimized.
This comment has been minimized.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
Doesn't jQuery have some accessor which does all this for us? |
This comment has been minimized.
This comment has been minimized.
|
... and wasn't jquery removed recently? |
steveklabnik
reviewed
Jun 30, 2015
| // Helper function for Keyboard events, | ||
| // Get's the char from the keypress event | ||
| // | ||
| // This method is used because e.wich === x is not |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I'm personally cool with not using jquery as much as possible. (jquery was removed from the book, not rustdoc) |
Gankro
reviewed
Jun 30, 2015
| if (e.shiftKey && $('#help').hasClass('hidden')) { | ||
| e.preventDefault(); | ||
| $('#help').removeClass('hidden'); | ||
| } | ||
| } else if (e.which === 27) { // esc | ||
| } else if (getChar(e) === 's' || getChar(e) === 'S') { |
This comment has been minimized.
This comment has been minimized.
Gankro
Jun 30, 2015
Contributor
A deep part of my soul wants these to be cached in a variable, but then I remember this is javascript and it doesn't matter at all.
This comment has been minimized.
This comment has been minimized.
From rustbook, not rustdoc.
I'm worried about the JS becoming a pile of impenetrable lists of platform-specific details for all browsers and weird combinations. I'd much rather have something else take care of all this so we don't have to keep updating little bits like this. |
This comment has been minimized.
This comment has been minimized.
Same, very much so. |
This comment has been minimized.
This comment has been minimized.
|
If jQuery has a method I haven't found it, and I didn't add jQuery, it was already there :) I am totally pro using jQuery, because they do a lot better job at implementing browser support and abstractions. If there is a jQuery way for this I'm all in, but I didn't find any unfortunately. I could drop the helper function and replace it with just String.fromCharCode( e.which )But I have no idea how it behaves in IE and I can't test it. |
This comment has been minimized.
This comment has been minimized.
|
@Gankro removing jQuery is in my opinion not really a win. Because it offers so much convenience methods to shorten and simplify the code. It's a lot easier to maintain than vanilla js. On top of that you get good browser compatibility removing / reducing the need to hack your way through it ;) Also jQuery is used on a lot of websites loaded from the official CDN, if we used that CDN jQuery would most likely already be cached in the users browsers and thus practically bring it's load time to zero. |
This comment has been minimized.
This comment has been minimized.
|
jquery is well-known to generally have significant overhead over "plain" DOM manipulations. Rustdoc being snappy is important to me. Especially as we make its render more dynamic in the future. |
This comment has been minimized.
This comment has been minimized.
|
That said, there's a reason I don't push hard for removing it ;) |
This comment has been minimized.
This comment has been minimized.
|
I am not totally convinced about the pure js vs jQuery performance. Sure jQuery has a bit of overhead because it has support for most browsers etc. But it is designed by a bunch of people who are a lot more skilled than myself and probably most contributors here, they probably know how to get that last bit of performance out of it. Well written vanilla js can be faster than jQuery but it has to be well written ;) One thing that comes to mind though is that it's a lot easier to use other css selectors instead of id's with jQuery because of the convenience methods, and this can drastically slow down the dom manipulations. I could be wrong, but I think we still have a pretty big marge before we begin to have performance issues in the rustdoc :) |
azerupi
referenced this pull request
Jun 30, 2015
Closed
Rust doc's question mark shortcut does not work well with all keyboard layouts. #16572
This comment has been minimized.
This comment has been minimized.
|
Hm ok, if there's nothing in jquery to already do this then it's probably fine. Can you squash the commits together as well? |
This comment has been minimized.
This comment has been minimized.
|
Done :) |
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Jul 1, 2015
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
merged commit 49b73e4
into
rust-lang:master
Jul 1, 2015
Gankro
added
the
beta-nominated
label
Jul 1, 2015
This comment has been minimized.
This comment has been minimized.
|
We should endeavour to back-port client-side rustdoc fixes as much as possible. |
aturon
added
the
T-tools
label
Jul 29, 2015
brson
removed
the
beta-nominated
label
Jul 29, 2015
This comment has been minimized.
This comment has been minimized.
|
Not accepted. |
azerupi commentedJun 30, 2015
Like explained in #26016, typing
?had no effect with non-english keyboard layouts in the docs.This patch seems to resolve this issue, tested with AZERTY keyboard in Google Chrome and Firefox. I haven't tested it with more exotic keyboard layouts or with other browsers though.
This code is based on the information found on: http://javascript.info/tutorial/keyboard-events
More specifically:
And
?andSwork,escapehowever does not (on an Azerty keyboard).It would be good if some people could test it with other browsers and keyboard layouts: http://www.mathieudavid.org/test/rustdoc/std/index.html
Edit: