-
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
Rewrite lints page #13269
base: master
Are you sure you want to change the base?
Rewrite lints page #13269
Conversation
@@ -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 |
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.
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
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.
What issue do you expect?
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.
If it uses the index.html
of the time but the latest deploy.sh
it would be missing the required lints.json
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.
Oh I see. An in-between situation.
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.
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 :)
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.
Just to confirm: did you forget "not" in "This should block improvements"? 😨
util/gh-pages/index_template.html
Outdated
<li role="separator" class="divider"></li> | ||
</ul> | ||
</div> | ||
<div class="btn-group", id="lint-applicabilities" tabindex="-1"> |
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.
Shouldn't be a comma here
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.
Arf, good catch!
util/gh-pages/index_template.html
Outdated
{% endif %} | ||
</div> |
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.
Fixed both DOM issues. |
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 Relatedly having to rerun |
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.
It's not really a big impact though. I don't expect many people will have this problem. |
To me that's not a worthwhile trade off |
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:
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 |
Personally I find the ctrl + F behaviour of
Sounds good
Seems like it would be a neutral change size wise if it's added to the JS, the menus are pretty small anyway |
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.
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.
Agreed, going to change it then (likely moving the theme-handling JS into a different file, loaded a the beginning).
This is a very good idea!
At least in firefox it doesn't expand
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. ;) |
Yes please, I think we should hit parity before landing it
I did my testing on a server with it hosted alongside the
Unfortunately chrome does |
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. |
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:
|
33a87a7
to
3dfb390
Compare
@Alexendoo Is there anything else to be done here? |
generateSettings(); | ||
generateSearch(); |
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.
Let's put these in the HTML from the start, it currently causes the page to move around a bit after a small delay
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 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).
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 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
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.
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?
Applied the suggestion for the HTML comment, for the rest, I can't reproduce the bugs locally... |
Fixed the settings button icon position. |
☔ The latest upstream changes (presumably #13384) made this pull request unmergeable. Please resolve the merge conflicts. |
3ad763c
to
044a780
Compare
Fixed merge conflict. |
☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts. |
…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.
044a780
to
025ba6e
Compare
Fixed (new) merged conflict. |
This PR has multiple goals:
r? @Alexendoo
changelog: make lint page work without web server