-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
HTML doc: navigate with left/right arrow keys #2064
HTML doc: navigate with left/right arrow keys #2064
Conversation
Adjusted basic theme's JS accordingly
Sure, why not. Is binding the arrow keys for this purpose used anywhere else? |
Sorry, I think I might not understand the question correctly, but let me try to answer:
|
Great idea! 👍 |
if (!$(document.activeElement).is('input')) { //don't navigate when in search box | ||
switch (event.keyCode) { | ||
case 37: //left | ||
var prevElement = $('link[rel="prev"]')[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be replaced by a call to $.prop('href')
Thanks for the feedback, I can follow/agree with all suggestions and will commit again later today or tomorrow. |
@birkenfeld , @lehmannro : Can this be merged eventually? |
I do not agree this feature by default. |
I think this behavior is not common. I've never seen such pages. |
Even if this may be an uncommon feature it will be helpful for users that know it exists. And it won't break any existing behavior. So it's primarily a question of people writing documentation to explain to their users that this feature exists and how it can be used. If someone uses a version of Sphinx that supports this feature and doesn't explain it nothing would break. Because it doesn't break anything I would not recommend to deactivate it by default and introduce another setting to activate it. Sphinx has already a lot of quite useful options. So adding new ones should be carefully thought of to avoid getting a longer and longer options list. |
initOnKeyListeners: function() { | ||
$(document).keyup(function(event) { | ||
var activeElementType = $(document.activeElement).prop('tagName'); | ||
if (activeElementType !== 'TEXTAREA' && activeElementType !== 'INPUT') { // don't navigate when in search box or textarea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that occurred to me: What about <select>
?
Also: Please move the comment to another line to satisfy 80 columns requirement.
I'm also in favor of merging this. I think we could easily make this configurable (move I'll leave it to you if you want to do this, or if somebody else should take on making it configurable. |
enable configuration via theme.conf
IMO, such behavior shouldn't be by default. I reluctantly agree if the feature is configurable and is disabled by default. |
This is a quite nice feature for those that expect it. However it may be really annoying for those that are not used to it and accidentally hits left/right. Also, it breaks the normal sideways scrolling behaviour. |
The navigation option is now non default and configurable in Anything else that should be changed? |
LGTM! Thanks. |
LGTM. To merge into master, @TimKam please rebase to resolve conflict. |
@shimizukawa It seems the confliction is already resolved. |
@tk0miya Sure! |
HTML doc: navigate with left/right arrow keys
Merged! |
Adjusted basic theme's JS accordingly
Resolves #1970
Does not navigate by design when focus is on input field, i.e. the search field