Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rewrite lints page #13269

Merged
merged 16 commits into from
Oct 11, 2024
Merged

Rewrite lints page #13269

merged 16 commits into from
Oct 11, 2024
+877 −894

Conversation

GuillaumeGomez
Copy link
Member

This PR has multiple goals:

  • Make lints page to work without needing a web server by removing the json file.
  • Prepare the field to also make the page work with JS (not done in this PR but should be straightforward).
  • Remove angular dependency.

r? @Alexendoo

changelog: make lint page work without web server

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 15, 2024
Alexendoo
Alexendoo reviewed Aug 16, 2024
View reviewed changes
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Fixed both DOM issues.

@Alexendoo
Copy link
Member

I've found that it's slower to load than the current site, not so much on my desktop but it's noticeable on my laptop/phone. There may be some value in keeping the lints.json method around so we can lazily create the lint bodies as requested

Relatedly having to rerun collect-metadata after any change to the template isn't great

@GuillaumeGomez
Copy link
Member Author

It's slower but since I'm aiming at having the website work even with JS disabled, we can't do much about it unfortunately. I have some tricks up my sleeve to reduce the page size by a lot though. I wanted to send follow-ups since this PR is already quite big.

Relatedly having to rerun collect-metadata after any change to the template isn't great

It's not really a big impact though. I don't expect many people will have this problem.

@Alexendoo
Copy link
Member

It's slower but since I'm aiming at having the website work even with JS disabled, we can't do much about it unfortunately

To me that's not a worthwhile trade off

@GuillaumeGomez
Copy link
Member Author

It's the usual: do you prefer users accessibility or users comfort? And in this case, since I plan to make more changes, it might not even be one or the other once I'm done since like I said, I have a lot of others changes lined up. Let me describe them so maybe it'll help you in your decision:

  1. Transform all lints into <details> so the JS to collapse/expand can be removed.
  2. Enable auto-whitespace stripping in rinja so that all rinja tags (like {% %}, {# #}...) will strip all whitespace surrounding them by default (more information here).
  3. Generate the menus and anything that require JS to work in JS directly like we do in rustdoc to reduce even further the page size by default.

And final argument: this website won't even need a server anymore once this PR is merged to work locally since the JSON file won't exist anymore. Meaning we can even distribute the website very easily and open it locally with cargo clippy doc or something like that.

@Alexendoo
Copy link
Member

What accessibility are we referring to here? Assistive technologies have no issue with elements created via JS

That aside, it was a response to us not being able to do much about it, if that's not the case then it's completely fine to ditch the JSON file

I've had a closer look and it's not solely down to the increase in transfer size/HTML parsing. With a couple changes it should be on par with master

  1. Make theme selection happen earlier (preferably inlined in <head>), currently the unthemed page becomes visible while the page is loading which makes the slowdown more noticeable
    image

  2. Make syntax highlighting lazy, a significant amount of the time is spent here upfront when it could happen only when the section is expanded

@Alexendoo
Copy link
Member

  1. Transform all lints into <details> so the JS to collapse/expand can be removed.

Personally I find the ctrl + F behaviour of <details> to be frustrating, auto expanding sections is not something I want it to do

  1. Enable auto-whitespace stripping in rinja so that all rinja tags (like {% %}, {# #}...) will strip all whitespace surrounding them by default (more information here).

Sounds good

  1. Generate the menus and anything that require JS to work in JS directly like we do in rustdoc to reduce even further the page size by default.

Seems like it would be a neutral change size wise if it's added to the JS, the menus are pretty small anyway

@GuillaumeGomez
Copy link
Member Author

What accessibility are we referring to here? Assistive technologies have no issue with elements created via JS

Mostly smaller devices like e-reader will have issues with JS. But in any case, it's bad practice to have the rendering done in two passes (one loading JS which in turn generates HTML) as it prevents to have information on the page directly.

I've had a closer look and it's not solely down to the increase in transfer size/HTML parsing. With a couple changes it should be on par with master

Yes, but that's something I wanted to do in another PR originally. Do you want me to do it in this PR directly? Also please note that when hosted on a web server (so not locally), all queries are compressed so the download time shouldn't be impacted much.

Make theme selection happen earlier (preferably inlined in ), currently the unthemed page becomes visible while the page is loading which makes the slowdown more noticeable

Agreed, going to change it then (likely moving the theme-handling JS into a different file, loaded a the beginning).

Make syntax highlighting lazy, a significant amount of the time is spent here upfront when it could happen only when the section is expanded

This is a very good idea!

Personally I find the ctrl + F behaviour of

to be frustrating, auto expanding sections is not something I want it to do

At least in firefox it doesn't expand <details> when using ctrl+f. I'm surprised any web browser would do it.

Seems like it would be a neutral change size wise if it's added to the JS, the menus are pretty small anyway

True. But I'll hide them if JS is disabled so at this point, why not just not generate them if the JS disabled. Less CSS required then. ;)

@Alexendoo
Copy link
Member

Yes, but that's something I wanted to do in another PR originally. Do you want me to do it in this PR directly?

Yes please, I think we should hit parity before landing it

when hosted on a web server (so not locally), all queries are compressed so the download time shouldn't be impacted much.

I did my testing on a server with it hosted alongside the master version, overall it transferred an extra ~30kb which yeah, is fine

At least in firefox it doesn't expand <details> when using ctrl+f. I'm surprised any web browser would do it.

Unfortunately chrome does

@GuillaumeGomez
Copy link
Member Author

Ok, going to do it another way (like I did in my blog for the "support me" button). But that's a task for another day anyway. Applying your suggestions in the meantime.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 29, 2024 •
edited
Loading

Moved the theme handling code into its own file, removed some inlined CSS, only run syntax highlighting when needed (when expanding lints) and greatly reduced the generated HTML page size:

before: 2.477.532 bytes
after:  1.777.286 bytes

@GuillaumeGomez
Copy link
Member Author

@Alexendoo Is there anything else to be done here?

Alexendoo
Alexendoo reviewed Sep 18, 2024
View reviewed changes
Cargo.toml Show resolved Hide resolved
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
util/gh-pages/style.css Show resolved Hide resolved
util/gh-pages/script.js Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Applied the suggestion for the HTML comment, for the rest, I can't reproduce the bugs locally...

@GuillaumeGomez
Copy link
Member Author

Fixed the settings button icon position.

@bors
Copy link
Contributor

bors commented Sep 21, 2024

☔ The latest upstream changes (presumably #13384) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed merge conflict.

@bors
Copy link
Contributor

bors commented Sep 22, 2024

☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts.

@GuillaumeGomez
Copy link
Member Author

Got an idea on how to prevent this: I simply default numbers in the badges. They get updated when the JS loads but at least the layout doesn't change. Does it look good to you @Alexendoo ?

Alexendoo
Alexendoo reviewed Sep 29, 2024
View reviewed changes
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
util/gh-pages/index_template.html Outdated Show resolved Hide resolved
util/gh-pages/script.js Outdated Show resolved Hide resolved
util/gh-pages/script.js Outdated Show resolved Hide resolved
util/gh-pages/script.js Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Applied your suggestions, thanks a lot for the thorough reviews. :)

@Alexendoo
Copy link
Member

Thanks, with a squash it looks good to go

@GuillaumeGomez
Copy link
Member Author

I squashed some changes and rewrote some parts of the git history so the changes can still be tracked independently. Or do you want just one commit?

@Alexendoo
Copy link
Member

That's 👍

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 6039343 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 6039343 with merge 6f1def7...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 6f1def7 to master...

@bors bors merged commit 6f1def7 into rust-lang:master Oct 11, 2024
8 checks passed
@GuillaumeGomez GuillaumeGomez deleted the rewrite-lints-page branch October 11, 2024 21:09
@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 11, 2024
Merged
@ageorgou ageorgou mentioned this pull request Oct 15, 2024
Merged
bors added a commit that referenced this pull request Oct 16, 2024
@bors
Auto merge of #13554 - ageorgou:doc-links, r=xFrednet
ccb30bf 
Documentation fixes

- In [the page describing the lints](https://rust-lang.github.io/rust-clippy/master/index.html), the View Source links are incorrect. This PR removes the extra segment causing them to fail.
  - Example before: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/clippy_lints/src/absolute_paths.rs#L13
  - Example after: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/absolute_paths.rs#L13
  - I think this was introduced in #13269.
  - I've only checked a few of the lints with the new template, but from what I can tell the metadata is generated in the same way for all of them. The `id_location` contains the full path of the lint declaration in the repository, which is why `clippy_lints` was repeated in the URL.
- Separately, fixing a typo in the explanation of `unnecessary_get_then_check`.

changelog: none
bors added a commit that referenced this pull request Oct 18, 2024
@bors
Auto merge of #13539 - GuillaumeGomez:allow-no-js, r=Alexendoo
6a79588 
Allow to go through clippy lints page without javascript

Fixes #13536.

This is the follow-up of #13269.

This PR makes it possible to expand/collapse lints (individually) without JS. To achieve this result, there are two ways:
1. Use `details` and `summary` tags. Problem with this approach is that the web browser search may open the `details` tags automatically if content matching it is inside. From a previous discussion with `@Alexendoo,` it seems to not be a desired behaviour.
2. Use a little trick where you use a `label` and a checkbox where the checkbox is in fact hidden. Then it's just a matter of CSS.

r? `@Alexendoo`

changelog: Allow to go through clippy lints page without JS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xFrednet xFrednet xFrednet left review comments

@flip1995 flip1995 flip1995 left review comments

@Alexendoo Alexendoo Alexendoo left review comments

Assignees

@Alexendoo Alexendoo

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @Alexendoo @bors @flip1995 @xFrednet @rustbot