Add settings menu on clippy lints page #13187
Conversation
rustbot
added
the
S-waiting-on-review
|
What is the use case for this? |
|
Maybe I didn't explain correctly, sorry about that. If you want the full context, you can see the mastodon discussion. |
|
Okay it doesn't break Tridactyl but it does break |
|
I think any plugin using |
|
I tried |
|
In rustdoc, we needed to implement this setting before we even had the |
|
Do we maybe want to move the |
|
I can do that and have a display similar to what we have in rustdoc. |
|
I don't like that / breaks native browser functionality (search in firefox) but if we decide to keep that I would be fine with an option If we drop that part I would want to see an example of something S breaks. I tried a few things but couldn't find an example (Tridactyl, Vimium, NVDA) |
|
|
|
I'm on Linux, last Firefox and Tridactyl 1.24.1 |
To clarify if you press But yeah since pretty much every Rust related site, GitHub and Zulip all have shortcuts on |
Yes, it opens a search bookmarks and history prompt.
Hmm... No? On GitHub and docs.rs I can disable the shortcuts. I'm not using Zulip as much, but there's a ticket about adding an option to disable shortcuts - zulip/zulip#16863 |
|
That isn't to say that we shouldn't add an option, just that you've experienced it breaking Interestingly crates.io behaves differently here, |
|
I still think that we should add this setting for users comfort and coherency with other rust platforms. |
|
As I said, I'm fine with keeping |
xFrednet
added
the
I-nominated
|
An idea, disabling the button for mobile devices, (e.g. width <= 768) as they usually have no keyboard. |
|
I will simply hide it in this case (as long as we only have this setting). Although, you can have keyboards on mobile devices. 😉 |
|
It was discussed on zulip and the conclusion was:
I'll push an update making this change. |
flip1995
removed
the
I-nominated
|
Both menus have been unified. I updated the screenshot in the first message. |
GuillaumeGomez
force-pushed
the
settings-menu
branch
from
8b148ac
to
8ef464f
Compare
|
Just checked, the new feature is responsive for mobile devices. |
| try { | ||
| localStorage.setItem(`clippy-lint-list-${settingName}`, value); | ||
| } catch (e) { } |
There was a problem hiding this comment.
Can drop the try/catch, not sure why it was there before but we aren't storing enough to hit any quota errors
There was a problem hiding this comment.
It's not because of quota but because localStorage might not be supported.
There was a problem hiding this comment.
Letting it throw would still be fine, we'd want to see the error in the console
Do we support any browsers where that's the case? I know way back when safari in private browsing behaved like that but that was fixed a long time ago also
It could happen on file: URLs but the site already doesn't work there due to the JSON file
There was a problem hiding this comment.
No it should definitely not fail. Not saving a setting is ok, crashing the script is not.
You can see the list of (not) supported platforms here. Basically, small web browsers used on some specific embed/mobile devices.
There was a problem hiding this comment.
Throwing in an event handler doesn't stop the script, it doesn't even unregister the handler
The grey is unknown not unsupported, the MDN sourced data isn't that thorough. A related entry from caniuse shows all the small ones support it with the exception of Opera Mini, but that's not relevant because it doesn't run JS
util/gh-pages/script.js
Outdated
| .directive('settingsDropdown', function ($document) { | ||
| return { | ||
| restrict: 'A', | ||
| link: function ($scope, $element, $attr) { | ||
| $element.bind('click', function () { | ||
| $element.toggleClass('open'); | ||
| $element.addClass('open-recent'); | ||
| }); | ||
|
|
||
| $document.bind('click', function () { | ||
| if (!$element.hasClass('open-recent')) { | ||
| $element.removeClass('open'); | ||
| } | ||
| $element.removeClass('open-recent'); | ||
| }) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
This and themeDropdown are unused now, but I think the dropdown should use this directive over the blur based one so it behaves the same as the other dropdowns on the page
Currently clicking on the Disable keyboard shortcuts label or inside the box but not on a control closes the popup
There was a problem hiding this comment.
This and
themeDropdownare unused now, but I think the dropdown should use this directive over the blur based one so it behaves the same as the other dropdowns on the page
I'm planning to make them follow the same logic (not using vue/angular) because they make the JS very difficult to debug to understand what's going on. It also allows to have less code and less JS events.
Currently clicking on the
Disable keyboard shortcutslabel or inside the box but not on a control closes the popup
Ah indeed, forgot a tabindex="-1" on the menu. Now it's behaving as expected, thanks!
GuillaumeGomez
force-pushed
the
settings-menu
branch
from
8ef464f
to
2ae6a49
Compare
|
Updated! |
|
@bors r+ The remaining things are pretty minor so let's not let it hold this up, I've been thinking about removing our usage of angular anyway since not many people are familiar with it (me included). We've slowly been gaining a bigger split between angular and not angular parts |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
I strongly agree and I'm working slowly towards this goal too. ;) |
It looks like this (when the menu is expanded):
Follow-up of #13178.
Someone pointed out that they should be able to disable the shortcuts on this page like it's the case for rustdoc and docs.rs. So here we go.
The first commit moves the style into its own file: it's much better for a web browser because it can then cache it.
The second one actually adds the new settings menu you can see above.
r? @Alexendoo
changelog: Add settings menu on clippy lints page