Improve CSS for source code block line numbers #143192
Conversation
rustbot
added
S-waiting-on-review
|
Some changes occurred in HTML/CSS/JS. |
There was a problem hiding this comment.
looks fine for the most part, just a few non-obvious things that could be clarified, and a few tiny edge cases that could hypothetically be improved, although I'm not sure if we care to, given how niche they are.
do we care about keeping git blame output clean? because rearranging things needlessly would degrade that slightly.
| .example-wrap.digits-1 { --example-wrap-digits-count: 1ch; } | ||
| .example-wrap.digits-2 { --example-wrap-digits-count: 2ch; } | ||
| .example-wrap.digits-3 { --example-wrap-digits-count: 3ch; } | ||
| .example-wrap.digits-4 { --example-wrap-digits-count: 4ch; } | ||
| .example-wrap.digits-5 { --example-wrap-digits-count: 5ch; } | ||
| .example-wrap.digits-6 { --example-wrap-digits-count: 6ch; } | ||
| .example-wrap.digits-7 { --example-wrap-digits-count: 7ch; } | ||
| .example-wrap.digits-8 { --example-wrap-digits-count: 8ch; } | ||
| .example-wrap.digits-9 { --example-wrap-digits-count: 9ch; } |
There was a problem hiding this comment.
Is this really the best way to do this? It very well may be "good enough", but here's some misc thoughts:
- pretty sure you can define
--variables in astyleattribute then have the styles of child elements reference them withvar. This way we could have the render logic pass an actual integer on to css instead of an integer-like enum. - if a rust file used the maximum possible file size and made each line use an average of 4 bytes or less, including newlines, it would have a 10 digit number of lines. Do we ever generate a
digits-10css class?
There was a problem hiding this comment.
- pretty sure you can define
--variables in astyleattribute then have the styles of child elements reference them withvar. This way we could have the render logic pass an actual integer on to css instead of an integer-like enum.
I'm not sure if I misunderstand what you mean or if you misunderstood what's going on in this code. Basically, example-wrap elements are everywhere, not just source code pages. Meaning we can have quite a wide number of lines (and of digits) around. I don't see how passing a variable to CSS could address this problem. Also, this is not an enum at all, just a class (digits-*) which is added by rustdoc when creating the <code>.
2. if a rust file used the maximum possible file size and made each line use an average of 4 bytes or less, including newlines, it would have a 10 digit number of lines. Do we ever generate a
digits-10css class?
I doubt a 10 billion lines source code will ever exist, so we should be on the safe side. Doing 1 billion is likely already far too much.
There was a problem hiding this comment.
Basically, example-wrap elements are everywhere, not just source code pages.
Yes, and instead of generating class="digits-N" we could generate style="--digits: N;", removing the need to have 9 different CSS rules.
Also, this is not an enum at all
It is an enum in the sense that it has a small number of possible values, each of which is handled separately.
I doubt a 10 billion lines source code will ever exist, so we should be on the safe side. Doing 1 billion is likely already far too much.
This is fair, i might send a followup PR showing what I mean (as it does feel like a nicer way to handle this), but it's most definitely not significant enough to block the current PR on.
There was a problem hiding this comment.
Yes, and instead of generating
class="digits-N"we could generatestyle="--digits: N;", removing the need to have 9 different CSS rules.
Having inline style should be avoided at all cost as it cannot be cached by the web browser.
Also, I don't see how it's better to declare CSS variable everywhere compared to have 9 CSS rules. The complexity increase seems way too much (for the web browser).
| @@ -8,6 +8,8 @@ | |||
| 3. Copy the filenames with updated suffixes from the directory. | |||
| */ | |||
|
|
|||
| /* ignore-tidy-filelength */ | |||
There was a problem hiding this comment.
This somewhat confuses me due to the fact that this file has actually gotten smaller, though it probably is correct to have here even if it isn't needed yet.
There was a problem hiding this comment.
It will be soon enough (like for #137229). We're already very close to the limit so better just allow it for everything to come.
GuillaumeGomez
force-pushed
the
code-line-number
branch
from
a9d5b05 to
d3b53a6
Compare
GuillaumeGomez
force-pushed
the
code-line-number
branch
from
d3b53a6 to
7c3bdda
Compare
|
Reduced the diff by moving the |
|
Thanks for the reviews! @bors r=lolbinary rollup |
bors
added
S-waiting-on-bors
Rollup of 9 pull requests Successful merges: - #143192 (Improve CSS for source code block line numbers) - #143251 (bootstrap: add build.tidy-extra-checks option) - #143273 (Make the enum check work for negative discriminants) - #143292 (Explicitly handle all nodes in `generics_of` when computing parent) - #143316 (Add bootstrap check snapshot tests) - #143321 (byte-addresses memory -> byte-addressed memory) - #143324 (interpret: move the native call preparation logic into Miri) - #143325 (Use non-global interner in `test_string_interning` in bootstrap) - #143327 (miri: improve errors for type validity assertion failures) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 9 pull requests Successful merges: - rust-lang/rust#143192 (Improve CSS for source code block line numbers) - rust-lang/rust#143251 (bootstrap: add build.tidy-extra-checks option) - rust-lang/rust#143273 (Make the enum check work for negative discriminants) - rust-lang/rust#143292 (Explicitly handle all nodes in `generics_of` when computing parent) - rust-lang/rust#143316 (Add bootstrap check snapshot tests) - rust-lang/rust#143321 (byte-addresses memory -> byte-addressed memory) - rust-lang/rust#143324 (interpret: move the native call preparation logic into Miri) - rust-lang/rust#143325 (Use non-global interner in `test_string_interning` in bootstrap) - rust-lang/rust#143327 (miri: improve errors for type validity assertion failures) r? `@ghost` `@rustbot` modify labels: rollup
Extract some changes from #137229 to make the PR smaller (thanks @yotamofek for the suggestion!).
r? notriddle