-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Generate scraped examples buttons in JS #130069
Conversation
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
7a31bc1
to
1ca3e90
Compare
if (!example.classList.contains("expanded")) { | ||
expandButton = createScrapeButton(buttonHolder, "expand", "↕"); | ||
} | ||
const isHidden = example.parentElement.classList.contains("more-scraped-examples"); |
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.
I noticed something while hunting through the code behind this variable.
Notice something wrong with this screenshot?
According to this code, the isHidden variable is actually used because the number of lines is supposed to be different depending on whether it's hidden or not. It looks like this was silently broken and nobody noticed.
Is this important, or should it just be simplified away?
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 is a question for @willcrichton. :)
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.
Is this an issue with this PR? If I glance at the Bevy docs, they seem to show the correct behavior:
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.
The link you gave is for 0.14.1. Try 0.14.2.
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.
I'm planning to send a follow-up PR to revert the height change since it's not linked to the current PR. Does it sound good to you @notriddle ?
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.
Yes. 👍
@bors r+ |
Ok, opening an issue then. @bors rollup |
☀️ Test successful - checks-actions |
Finished benchmarking commit (749f80a): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 769.055s -> 769.206s (0.02%) |
Follow-up of #129796.
To reduce the page size when there are scraped examples, we can generate their buttons in JS since they require JS to work in any case. There should be no changes in display or in functionality.
You can test it here.
cc @willcrichton
r? @notriddle