-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Fix anchor links and add spinner to the benchmarks page #2205
Conversation
I fixed the A minor change was required to the |
website/benchmarks.html
Outdated
@@ -153,10 +156,25 @@ <h3 id="syscalls">Syscall count <a href="#syscalls">#</a></h3> | |||
<script type="module"> | |||
import { drawCharts } from "./app.js"; | |||
window.chartWidth = 800; | |||
const busy = document.getElementById("busy") |
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 think we should name it more component-like way, like spinner-wrapper
, spinner-overlay
, or spinner-area
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.
Sure spinner-overlay
sounds best to me.
website/benchmarks.html
Outdated
} | ||
|
||
function updateCharts () { | ||
let u = window.location.hash.match("all") ? "./data.json" : "recent.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.
How about making u
const
?
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 agree (I didn't add that line).
I see a lot of js code using let
where const
could (should ?) be used.
I always favour using const
and rarely use let
but others may disagree.
I'll make the change.
Can you add the gif of this change so that people can (easily) review it aesthetically? |
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.
Thanks for the updates!
LGTM
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.
LGTM - thanks!
Thanks! |
Issue: #2203