Improve styles of filtering options for Clippy's lint list #8070

CrazyRoka · 2021-12-03T13:15:19Z

Partially solves #7958

Updated styles for filtering options. It now uses dropdown menus.

changelog: none

rust-highfive · 2021-12-03T13:15:22Z

(rust-highfive has picked a reviewer for you, use r? to override)

camsteffen · 2021-12-03T18:50:15Z

The dropdowns look really nice! I think there should be some sort of always-visible indicator of what is selected. Possibly just the number of selected items, or maybe even just a different style to indicate that the default selection has been changed. My main concern being that it's easy to forget that you are not looking at a complete list.

CrazyRoka · 2021-12-04T17:48:06Z

I added badges to display selected items count. Have a look:

camsteffen · 2021-12-05T15:12:11Z

Nice. I think the badge could be the same color as the lint category badges. Also can we see what it looks like on the right side of the button?

CrazyRoka · 2021-12-05T18:11:07Z

I updated the code. Have a look:

Badge color is now #777
Badge is now located at the right side

xFrednet · 2021-12-05T18:13:07Z

Also, a +1 from me for the dropdown menus, they look really nice 👍

The website also supports the different color themes from mdBook. The change theme button is in the upper-left corner. Can you make sure that your changes also support the change of color? You can find the available css values here: https://github.com/rust-lang/mdBook/blob/master/src/theme/css/variables.css

CrazyRoka · 2021-12-05T20:17:25Z

@xFrednet @camsteffen Have a look on this video:

ClippyLints.mp4

xFrednet · 2021-12-05T21:56:16Z

I really like it, nice work! 👍

Also cc: @flip1995 since you've suggested this tabbed filter layout. 🙃

camsteffen · 2021-12-05T23:18:19Z

I am seeing JavaScript errors upon selecting options.

The button style seems to change from selected to deselected after clicking one of the options. Maybe this could be fixed with a CSS rule? If not it's probably fine.

CrazyRoka · 2021-12-06T09:49:42Z

I am seeing JavaScript errors upon selecting options.

I could see the error in Firefox browser, but not in Chrome. Replaced invalid code with correct one.

The button style seems to change from selected to deselected after clicking one of the options. Maybe this could be fixed with a CSS rule? If not it's probably fine.

There was a style from bootstrap. I replaced it with custom one. Now selected drop-down should be slightly darker than others.

camsteffen · 2021-12-06T16:56:49Z

I think it would be wise to use the Bootstrap dropdown js plugin (link), especially since we're already using Bootstrap CSS. This would be an added dependency though.

@rust-lang/clippy Everyone good with the general direction here?

flip1995 · 2021-12-06T17:13:36Z

I like the general approach and how the drop down menus look. I don't know anything about JS / CSS, so I can't really review or give opinions on that front.

Only concern is, if it works and how it looks on mobile.

flip1995

Selecting "All" has a small lag. This was especially noticeable on mobile.

Here's a link to a deployed version of this branch (rebased on current master): https://flip1995.github.io/rust-clippy/master/index.html

One issue I found on mobile is, that the color scheme drop down is behind the other drop downs. Otherwise it LGTM on mobile as well.

util/gh-pages/index.html

camsteffen · 2021-12-06T18:30:19Z

The "Filter" field could be on the same line and wrap responsively.

CrazyRoka · 2021-12-06T22:06:05Z

Selecting "All" has a small lag. This was especially noticeable on mobile.

I can see small lag on my modern laptop. Looks like selecting "All" will render all the lints at once and it causes Angular to take time building the DOM and parsing html:

We can add pagination to prevent this

CrazyRoka · 2021-12-06T22:35:08Z

The "Filter" field could be on the same line and wrap responsively.

Implemented this one.

One issue I found on mobile is, that the color scheme drop down is behind the other drop downs.

Fixed this one

I think it would be wise to use the Bootstrap dropdown js plugin (link), especially since we're already using Bootstrap CSS. This would be an added dependency though.

Does it support multi-select? As you can see, current dropdown supports selecting multiple entries. Also, it doesn't close dropdown, so user can select multiple items and close it by clicking on the button or somewhere in the page.

camsteffen · 2021-12-06T23:44:32Z

Another thing we could do about the lag is make the lint panel expanded contents render lazily with something like ngIf="expanded". That could be a standalone enhancement.

Does it support multi-select? As you can see, current dropdown supports selecting multiple entries. Also, it doesn't close dropdown, so user can select multiple items and close it by clicking on the button or somewhere in the page.

I'm only judging by the docs, but I think the dropdown plugin is pretty unopinionated - you can put any HTML in the dropdown menu and you can close it programmatically like you currently are (but I would try out the default behavior first).

flip1995 · 2021-12-07T09:28:57Z

Redeployed: https://flip1995.github.io/rust-clippy/master/index.html

Another thing I noticed on mobile (tangentially related to this PR): The "Fork me on GitHub" banner in the top right corner slightly overlaps with the Lint groups dropdown. So when you try to open the dropdown by pressing on the error it redirects you to the Clippy GH repo, instead of opening the dropdown.

CrazyRoka · 2021-12-07T16:45:24Z

Another thing I noticed on mobile (tangentially related to this PR): The "Fork me on GitHub" banner in the top right corner slightly overlaps with the Lint groups dropdown. So when you try to open the dropdown by pressing on the error it redirects you to the Clippy GH repo, instead of opening the dropdown.

Fixed this issue with clip-path property. So, clickable area would look like this:

CrazyRoka · 2021-12-07T17:44:12Z

Another thing we could do about the lag is make the lint panel expanded contents render lazily with something like ngIf="expanded". That could be a standalone enhancement.

Performance was improved. As you can see, Browser computation is now taking the most time after click:

CrazyRoka · 2021-12-08T13:43:11Z

@flip1995 @camsteffen @xFrednet I went through all the reviews, and it looks like all the issues are resolved. Let me know if I missed something.
I haven't used the dropdown plugin, so. The reason is that I implemented this with raw ng-click, but this plugin will require me to rewrite it in jquery and add additional code to the rendered HTML.
@camsteffen If you strongly suggest using the plugin instead, I will add it.

camsteffen · 2021-12-08T16:46:47Z

Oh I didn't realize it requires jQuery. That's fine then. I hate jQuery lol. I still need to do a full review but I feel confident about this PR overall.

util/gh-pages/index.html

xFrednet · 2021-12-08T19:05:18Z

The performance increase while searching is wonderful. I really like the changes!

flip1995

This looks really good now. I love the perf improvement. Thank you for addressing all of our reviews so quickly!

Before this can be merged, please rebase your commits, in order to get rid of the merge commit.

CrazyRoka · 2021-12-09T12:01:23Z

This looks really good now. I love the perf improvement. Thank you for addressing all of our reviews so quickly!

Before this can be merged, please rebase your commits, in order to get rid of the merge commit.

I rebased my commits and added one new change:

Theme menu now should be closed when interacting with the page
Dropdowns are now implemented with directives (less code, more maintainability)
Removed raw js handler for theme menu

camsteffen

How about aligning the text and checkboxes like this?

Feel free to clean up the commit history a bit as well.

util/gh-pages/index.html

camsteffen · 2021-12-14T15:17:50Z

util/gh-pages/index.html

+            color: var(--searchbar-fg);
+            border-color: var(--theme-popup-border);
+            filter: brightness(90%);
+        }


Bootstrap theming is normally done by downloading bootstrap-theme.css and modifying the values. We don't need an entire theme, but I think we should copy parts of that code, and add a comment saying as much.

These variables are coming from rust mdBook styles. Which code do you mean?

You can get bootstrap-theme.css from the "Download Bootstrap" link here.

From there you can copy and modify the following CSS rule, for example. Ideally all theming can be done this way.

/* Bootstrap Theme - rules copied from bootstrap-theme.css with modified values */ .btn-default:active, .btn-default.active { - background-color: #e0e0e0; - border-color: #dbdbdb; + background-color: var(--searchbar-bg); + border-color: var(--theme-popup-border); }

CrazyRoka · 2021-12-16T17:05:44Z

@camsteffen I reviewed your suggestions and applied them. One comment is still under review, have a look. Let me know if you have any other concerns.

camsteffen · 2022-01-05T22:55:08Z

@CrazyRoka just checking in. There's still one open thread. But honestly it's not super important and we can just wrap this up if you prefer.

xFrednet · 2022-03-09T19:11:09Z

Hey @camsteffen, do we want to merge it, since @CrazyRoka hasn't replied yet? I like these changes and would like to have them merged before they conflict with new changes 🙃

camsteffen · 2022-03-10T03:07:10Z

Okay. @bors r+

bors · 2022-03-10T03:07:11Z

📌 Commit 296af08 has been approved by camsteffen

bors · 2022-03-10T03:07:18Z

⌛ Testing commit 296af08 with merge b4cfdcd...

…msteffen Improve styles of filtering options for Clippy's lint list Particularly solves #7958 Updated styles for filtering options. It now uses dropdown menus. ![image](https://user-images.githubusercontent.com/19844144/144608479-cdd9de0b-f101-4d49-a135-0969efb01a11.png)

bors · 2022-03-10T03:07:52Z

💔 Test failed - checks-action_test

camsteffen · 2022-03-10T03:08:30Z

@bors retry

bors · 2022-03-10T03:08:37Z

⌛ Testing commit 296af08 with merge 8ae74da...

bors · 2022-03-10T03:25:51Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: camsteffen
Pushing 8ae74da to master...

rust-highfive assigned camsteffen Dec 3, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 3, 2021

flip1995 reviewed Dec 6, 2021

View reviewed changes

util/gh-pages/index.html Outdated Show resolved Hide resolved

xFrednet reviewed Dec 8, 2021

View reviewed changes

util/gh-pages/index.html Outdated Show resolved Hide resolved

flip1995 approved these changes Dec 9, 2021

View reviewed changes

CrazyRoka force-pushed the update-gh-pages-checkbox-styles branch from 0a54144 to 2eaa17e Compare December 9, 2021 11:51

camsteffen reviewed Dec 14, 2021

View reviewed changes

CrazyRoka added 10 commits December 16, 2021 18:57

Updated styles for checkboxes

a71f199

Added badge to display selected elemenets count

0f655db

Updated badge

cb113d3

Fixed styles on all themes

f71039a

Fixed error in Firefox and selected button style

e895d2b

Fixed theme selector and repositioned search input

76e95e2

Fixed overlapping "Fork me" logo

dcfb756

Improved performance by adding ng-if

bb53132

Added dropdown directive to theme-selector

4161a67

Removed unused styles and labels in dropdowns

296af08

CrazyRoka force-pushed the update-gh-pages-checkbox-styles branch from 5e42d1e to 296af08 Compare December 16, 2021 17:01

camsteffen added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 27, 2021

camsteffen mentioned this pull request Jan 14, 2022

Update markdown-it version #8274

Merged

xFrednet mentioned this pull request Mar 9, 2022

"Clippy Lints" page forgets "Lint groups" selection #8510

Closed

bors merged commit 8ae74da into rust-lang:master Mar 10, 2022

Improve styles of filtering options for Clippy's lint list #8070

Improve styles of filtering options for Clippy's lint list #8070

Conversation

CrazyRoka commented Dec 3, 2021 • edited by camsteffen

rust-highfive commented Dec 3, 2021

camsteffen commented Dec 3, 2021 • edited

CrazyRoka commented Dec 4, 2021

camsteffen commented Dec 5, 2021

CrazyRoka commented Dec 5, 2021

xFrednet commented Dec 5, 2021

CrazyRoka commented Dec 5, 2021

xFrednet commented Dec 5, 2021

camsteffen commented Dec 5, 2021

CrazyRoka commented Dec 6, 2021

camsteffen commented Dec 6, 2021

flip1995 commented Dec 6, 2021 • edited

flip1995 left a comment

Choose a reason for hiding this comment

camsteffen commented Dec 6, 2021

CrazyRoka commented Dec 6, 2021

CrazyRoka commented Dec 6, 2021

camsteffen commented Dec 6, 2021

flip1995 commented Dec 7, 2021

CrazyRoka commented Dec 7, 2021

CrazyRoka commented Dec 7, 2021

CrazyRoka commented Dec 8, 2021

camsteffen commented Dec 8, 2021

xFrednet commented Dec 8, 2021

flip1995 left a comment

Choose a reason for hiding this comment

CrazyRoka commented Dec 9, 2021

camsteffen left a comment

Choose a reason for hiding this comment

camsteffen Dec 14, 2021

Choose a reason for hiding this comment

CrazyRoka Dec 16, 2021

Choose a reason for hiding this comment

camsteffen Dec 16, 2021

Choose a reason for hiding this comment

CrazyRoka commented Dec 16, 2021

camsteffen commented Jan 5, 2022

xFrednet commented Mar 9, 2022

camsteffen commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

camsteffen commented Mar 10, 2022

bors commented Mar 10, 2022

bors commented Mar 10, 2022

CrazyRoka commented Dec 3, 2021 •

edited by camsteffen

camsteffen commented Dec 3, 2021 •

edited

flip1995 commented Dec 6, 2021 •

edited