-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add separate about.html and instructions.html #359
Add separate about.html and instructions.html #359
Conversation
Is it ready for review or are you waiting for the document? |
Let's try to land this already so everything is in place and we only need to update the content. |
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.
looks good to me, thanks
make sure to run npm run format
before landing.
I noticed one issue, but I believe it's unrelated to this patch, rather related to one of the setLocationHash patches:
- Start the benchmark.
- Before the benchmark is finished, press F5.
=> empty page
Can you please look at this issue?
I forgot to redirect the #running view to #home by default if there are no results yet. |
I know it was added for a reason, but I wonder if we should reconsider the hash navigation. It seems to me the benefit would be if we wanted to allow users to drive UI states with the back button - and in general we don't, in the sense that you can't go back from #summary to #running by design. If we don't need that property there would be simpler ways to manage the UI state that avoid these types of bugs: just set some kind of attribute on the body to toggle display for the corresponding section (or even toggling an attribute on the sections themselves). One thing that's nice about the current behavior is that you can deep link to the About dialog, but we could handle that as a special case (reading location.hash at page load and toggling the about dialog if it matches "#about"). Any thoughts on this @camillobruni @rniwa ? Not volunteering Camillo to make that change, and I don't think it's a blocker, but if we generally agree on this then maybe @julienw or I could work on a patch if you'd like. |
In addition to the good suggestion from bgrins, I was also thinking that all the instructions and about texts could be in a separate plain HTML page, so that the runner would stay fairly simple with its 4 states (home / running / details / summary). |
Yeah, that could be a simpler solution overall. |
I agree a separate page may be a bit simpler, but the test instructions are quite contextual and make at least as much sense in the main UI as About does (IMO). But if we do want to move it to a new page, I'd suggest we make a markdown file in GitHub and link to it so that (a) we don't have to figure out styling for the new page and (b) we can update the text "off-train" and not require a version bump if we want to include a new instruction. |
index.html
Outdated
<ul> | ||
<li>Close all other applications.</li> | ||
<li>Close other browser windows and tabs.</li> | ||
<li>Use a fresh browser profile and a single window.</li> |
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.
Maybe we want to explicitly call out that browser extensions should be disabled?
I'm fine either way. |
I've now moved the about and instructions sections into standalone .html files which makes the navigation logic much simpler. |
Adds a basic instructions section.
To be extended once the document in #352 is filled in.