Rewrite lints page #13269

GuillaumeGomez · 2024-08-15T14:56:33Z

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

…dering

util/gh-pages/index_template.html

Alexendoo · 2024-08-15T16:28:02Z

.github/deploy.sh

@@ -9,7 +9,6 @@ echo "Making the docs for master"
 mkdir out/master/
 cp util/gh-pages/index.html out/master
 cp util/gh-pages/script.js out/master
-cp util/gh-pages/lints.json out/master


With

rust-clippy/.github/workflows/deploy.yml

Line 48 in cbf019b

git checkout origin/master -- .github/deploy.sh util/versions.py util/gh-pages/versions.html

Would this create an issue with deploying the 1.81 docs? cc @flip1995

What issue do you expect?

If it uses the index.html of the time but the latest deploy.sh it would be missing the required lints.json

Oh I see. An in-between situation.

Yeah, we probably have to push old versions of the html files to the gh-pages` branch manually for the 2 releases following this.

Thanks for the ping! This should block improvements here though. I figured something similar out before and am optimistic to be able to do that again :)

Just to confirm: did you forget "not" in "This should block improvements"? 😨

Alexendoo · 2024-08-16T15:13:40Z

util/gh-pages/index_template.html

+                                <li role="separator" class="divider"></li>
+                            </ul>
+                        </div>
+                        <div class="btn-group", id="lint-applicabilities" tabindex="-1">


Shouldn't be a comma here

Arf, good catch!

Alexendoo · 2024-08-16T15:19:02Z

util/gh-pages/index_template.html

+                            {% endif %}
+                            </div>


Suggested change

{% endif %}

</div>

</div>

{% endif %}

GuillaumeGomez · 2024-08-16T15:33:06Z

Fixed both DOM issues.

Alexendoo · 2024-08-26T15:04:06Z

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 · 2024-08-26T15:07:13Z

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 · 2024-08-26T15:13:30Z

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 · 2024-08-26T15:20:23Z

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:

Transform all lints into <details> so the JS to collapse/expand can be removed.
Enable auto-whitespace stripping in rinja so that all rinja tags (like {% %}, {# #}...) will strip all whitespace surrounding them by default (more information here).
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 · 2024-08-29T15:12:26Z

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

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
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 · 2024-08-29T16:21:20Z

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

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

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 · 2024-08-29T17:56:45Z

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 · 2024-08-29T19:04:18Z

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 · 2024-08-29T19:14:09Z

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.

…before first rendering

Since all angular code was removed, there is no conflict anymore before angular and jinja2 syntaxes so we can just use plain jinja2 syntax.

GuillaumeGomez · 2024-08-29T19:45:56Z

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 · 2024-09-17T19:14:25Z

@Alexendoo Is there anything else to be done here?

Alexendoo · 2024-09-11T12:08:16Z

Cargo.toml

@@ -39,6 +39,8 @@ toml = "0.7.3"
 walkdir = "2.3"
 filetime = "0.2.9"
 itertools = "0.12"
+pulldown-cmark = "0.11"
+rinja = { version = "0.3", default-features = false, features = ["config"] }


A high level question - is there a particular reason we're going for this templating framework rather than a more common one? I had not heard of it before

You might have heard of askama. I contributed to it a lot but some issues happened there (nothing bad, just different opinion on some specific things) and another askama maintainer and I decided to fork it.

You can see more information about it here.

The biggest advantage of rinja is that if the template code is wrong, it will prevent compilation (which is the big reason why we moved to this framework for docs.rs).

util/gh-pages/index_template.html

Alexendoo · 2024-09-11T13:43:37Z

util/gh-pages/style.css

-  padding-left: 4px;
-  padding-top: 3px;


Looks unintentional, this moves the settings cog off-centre

Strangely enough, if I put it back I get:

It is more noticeable in chrome (the UA stylesheet for button padding is different)

But the image is slightly below centre in firefox also, before/after:

Hum. Let me check if I can unify this across browsers.

Instead of having something set by the default, I now force its position by using absolute positioning. I checked on firefox and chrome and both look the same now.

Alexendoo · 2024-09-18T20:18:02Z

util/gh-pages/script.js

+generateSettings();
+generateSearch();


Let's put these in the HTML from the start, it currently causes the page to move around a bit after a small delay

This script is downloaded in a non-async method before the DOM is rendered, so there should not be any layout re-rendering (also I can't reproduce the bug locally, even with throttling).

It is in a regular script tag before the end of the document, but browsers do not wait for the document to be complete before they begin rendering

When exactly browsers incrementally render varies but here it happens to be more pronounced in chrome, it's still visible as a flicker in firefox though. It's most noticeable when the size happens to cause a line break e.g. in firefox with disable cache ticked and 3G throttling:

Desktop.2024.09.19.-.14.54.53.07.mp4

Good catch! The current lint page (before this PR) is displaying everything once it's loaded, so a different kind of "layout break". Seems like it's part of what I want to do as a follow-up: render buttons and any JS-bound element with JS instead of keeping it into HTML. Do you want me to do it in this PR directly or as a follow-up like I originally planned?

GuillaumeGomez · 2024-09-18T22:11:46Z

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

GuillaumeGomez · 2024-09-19T13:52:15Z

Fixed the settings button icon position.

bors · 2024-09-21T07:35:06Z

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

GuillaumeGomez added 13 commits August 15, 2024 16:40

Generate lint list in HTML directly instead of JS

61d407a

Update cargo dev serve command to look over the correct files

fe268de

Replace search with vanilla JS

4032b3f

Finish porting lint functionalities to vanilla JS

f453739

Completely remove angular and generate parts of settings in JS

cf98701

Bring back dropdown settings menus

b8169ef

Put back interactions with settings menus

839787e

Support all filters except versions

6307ea4

Add support for version filtering

7d421b8

Add support for URL arguments

9eec5eb

Support version filter URL parameters

99e0318

Improve rendering speed by moving settings generation after theme ren…

449f827

…dering

Update settings to follow recent master changes

2daa5b7

rustbot assigned Alexendoo Aug 15, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 15, 2024

GuillaumeGomez added 2 commits August 15, 2024 16:57

Fix fmt

7980941

Fix dogfood error

cbf019b

xFrednet reviewed Aug 15, 2024

View reviewed changes

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

Alexendoo reviewed Aug 15, 2024

View reviewed changes

Set correct URL in clippy lints HTML comment

76a08df

Alexendoo reviewed Aug 16, 2024

View reviewed changes

GuillaumeGomez added 4 commits August 29, 2024 21:44

Move theme handling JS into its own file to make theme being applied …

d67093d

…before first rendering

Replace rinja custom syntax with default syntax

00ce749

Since all angular code was removed, there is no conflict anymore before angular and jinja2 syntaxes so we can just use plain jinja2 syntax.

Move inlined CSS into style.css

543734f

Run syntax highlighting only when needed

0c69226

Greatly reduce generated HTML page size

3dfb390

GuillaumeGomez force-pushed the rewrite-lints-page branch from 33a87a7 to 3dfb390 Compare August 29, 2024 19:47

Alexendoo reviewed Sep 18, 2024

View reviewed changes

Replace jinja comment with HTML comment

cf4934f

Fix settings button icon position

3ad763c

		generateSettings();
		generateSearch();

Rewrite lints page #13269

Are you sure you want to change the base?

Rewrite lints page #13269

Conversation

GuillaumeGomez commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Aug 16, 2024

Alexendoo commented Aug 26, 2024

GuillaumeGomez commented Aug 26, 2024

Alexendoo commented Aug 26, 2024

GuillaumeGomez commented Aug 26, 2024

Alexendoo commented Aug 29, 2024

Alexendoo commented Aug 29, 2024

GuillaumeGomez commented Aug 29, 2024

Alexendoo commented Aug 29, 2024

GuillaumeGomez commented Aug 29, 2024

GuillaumeGomez commented Aug 29, 2024 • edited Loading

GuillaumeGomez commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Sep 18, 2024

GuillaumeGomez commented Sep 19, 2024

bors commented Sep 21, 2024

GuillaumeGomez commented Aug 29, 2024 •

edited

Loading