-
Notifications
You must be signed in to change notification settings - Fork 531
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
Add ability to disable website keybindings for show/hide controller (Fixes: #526) #533
Add ability to disable website keybindings for show/hide controller (Fixes: #526) #533
Conversation
This looks nice, thanks for the work. But I am left wondering why show/hide is treated differently from every other feature. I would prefer a small refactor from the original to just treat show/hide like everything else. It seems like this was explicitly not done in #350. Do you have any ideas why? @igrigorik @canarslan12 |
@jacobcolbert scratching my head as well, I think this was an oversight as I don't recall any specific reason why we decided to specifically omit this feature. Also, +1 to your recommendation on refactor. |
In terms of external behavior, yes that is exactly what I mean. In the code I mean that show/hide is treated by itself as an exceptional case while every other feature is treated uniformly. Specifically, in options.html it is implemented as The code should treat all of these features the same, so that if we change things in the future it will automatically affect all of them equivalently. |
Nice! I think this looks good now, nice touch on custom bindings carryover.. @jacobcolbert any feedback before we merge? |
Looks great to me. Gonna fix some indentation errors that were already there. Very nice touch on carrying over the custom bindings. Would prolly have missed that myself. Can that code be removed in the next version after this is pushed live? |
Hmmm that's a good question. I think the part in Lines 93 to 101 in 29788ba
It is similar to the current code to initialize the other shortcuts Lines 26 to 71 in 29788ba
The Then we can remove this line Lines 187 to 190 in 29788ba
And also this following one as Lines 197 to 199 in 29788ba
|
Demo
![show-hide](https://user-images.githubusercontent.com/892961/66641314-0f9b8c80-ec4d-11e9-8b04-57a2ba26523c.gif)
Demo of it showing only after clicking "show experimental features" and of it being able to persist on saving