-
Notifications
You must be signed in to change notification settings - Fork 15
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
Address single page view format issues #944
Conversation
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.
@Paul-Clue looks good and happy you're so close to finishing off this bit of work!
There is something key missing, which is with handling assertionExceptions
for the rendered commands tables.
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Outdated
Show resolved
Hide resolved
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Outdated
Show resolved
Hide resolved
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Outdated
Show resolved
Hide resolved
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Outdated
Show resolved
Hide resolved
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Show resolved
Hide resolved
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.
@Paul-Clue LGTM! Thanks for addressing my feedback
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.
@Paul-Clue apologies, looking back at the feedback of #745 (comment).
These items have not been fully addressed:
- Under the heading "About This Test Plan", the title of the latest commit is not a link; it should be a link just like it is on the test plan version history page. Editorial note: I think it would be better if that list item had content: "Latest commit: title of latest commit.
- There is an information architecture problem that does not appear to be covered by the original specification. The tests are under an H2 of "Supporting Documentation" instead of under the H2 "Tests". The "Supporting Documentation" heading should be moved before the ""Tests" heading. The resulting H2 order would then be: "About This Test Plan", "Supporting Documentation", "Tests". Then, each individual test will be an H3 under the H2 of "Tests".
client/components/CandidateReview/CandidateTestPlanRun/InstructionsRenderer.jsx
Outdated
Show resolved
Hide resolved
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.
Hi @Paul-Clue looks great, I was able to verify it working and successfully tested tons of test plans, on both the data management section as well as the candidate review section. I just have one possible issue and a couple of quick comments.
let shouldCount = 0; | ||
let mayCount = 0; | ||
|
||
let assertions = [...renderableContent.assertions]; |
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 ( | ||
settingsScreenText === null || | ||
settingsScreenText === '' | ||
) { |
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.
Both of these values are falsy so this could be if (!settingsScreenText) {
or even more compact:
const settingsScreenTextFormatted = settingsScreenText ? ` (${settingsScreenText})` : ''
Looking much better!! Thank you!! I see one important problem looking at the page for alert in staging: Captions on the table of assertions under each command heading are incorrect for NVDA and VoiceOver. For example, the caption on the spacebar command for voiceOver says:
It seems the caption is coming from the first AT, which is JAWS. |
In the toggle button plan, there is no caption on the table for the first command for each screen reader. Of less concern, in a v1 plan, like menu button, the captions say "Results for COMMAND_NAME" instead of following the caption format based on prior heading like it should. |
Tested in staging. All resolved. Beautiful work. Thank you! |
see issue #745
This PR addresses the second set formatting issues brought up in issue #745.