-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 settings menu on clippy lints page #13187
Conversation
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 |
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. |
Both menus have been unified. I updated the screenshot in the first message. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not because of quota but because localStorage
might not be supported.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 shortcuts
label 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!
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