Implement Test Review in React #846
Conversation
There was a problem hiding this comment.
@alflennik looks good! There was a lot of "cleanup" here in terms of references and code and it seems you've gotten to it all.
I've left some inline comments on minor (I think) changes.
| aria-current={location.pathname.startsWith( | ||
| '/data-management' | ||
| )} | ||
| aria-current={ |
There was a problem hiding this comment.
Thanks for this attention to detail!
| rel="noreferrer" | ||
| target="_blank" | ||
| > | ||
| {title} |
There was a problem hiding this comment.
This produces link text such as example, aria-live, etc which is different from what we have now being APG example: <pattern>.html, ARIA specification: aria-live, etc.
I think this is fine for now given how the app imports those tests, but just calling out that this will be extended in the future to support clearly defined link text for v2 test format tests.
There is no request for a change here, just a note.
| </li> | ||
| <li> | ||
| <strong>Version History: </strong> | ||
| <Ul> |
There was a problem hiding this comment.
This doesn't cover the following condition from #788:
If current phase is R&D Complete, add list item with string "This version is a proposed draft; ARIA-AT review processes have not yet started."
There was a problem hiding this comment.
Ah right, I actually noticed a contradiction in between the spec and the example so I decided to go with the text the example gave, which is "R&D completed on Apr 14, 2022 for this version." Do you think that's ok?
There was a problem hiding this comment.
Okay, understood and yep, seems okay to me.
| <p> | ||
| {`This page contains the full content of all ${testCount} ` + | ||
| `tests the ARIA-AT team has authored for ` + | ||
| `${testPlanVersion.title} as of version ` + |
There was a problem hiding this comment.
| `${testPlanVersion.title} as of version ` + | |
| `${testPlanVersion.title}, version ` + |
This read to me like newly added tests, in some future VYY.MM.DD would and could also show up for the pattern, on the 'current' version's page.
There was a problem hiding this comment.
Sure this is fine!
| </h1> | ||
| <h2>Introduction</h2> | ||
| <p> | ||
| {`This page contains the full content of all ${testCount} ` + |
There was a problem hiding this comment.
| {`This page contains the full content of all ${testCount} ` + | |
| {`This page contains ${testCount} ` + |
| @@ -202,8 +193,10 @@ const InstructionsRenderer = ({ testResult, testPageUrl, at }) => { | |||
|
|
|||
| const assertionsContent = parseListContent(assertions); | |||
|
|
|||
| const Heading = `h${headingLevel}`; | |||
There was a problem hiding this comment.
I didn't know that you could create HTML elements with strings in JSX like this. Very neat.
There was a problem hiding this comment.
Hah yeah neat is like the perfect word for it
| @@ -553,7 +553,7 @@ const DataManagementRow = ({ | |||
| <VersionString | |||
| role="listitem" | |||
| iconColor="#2BA51C" | |||
| linkHref={`/test-review/${latestVersion.gitSha}/${latestVersion.testPlan.directory}`} | |||
| linkHref={`/test-review/${latestVersion.id}`} | |||
There was a problem hiding this comment.
It seems like the app isn't so heavily used or linked to that this will matter but just putting a note here to make sure that we all agree that breaking previous sha-based routing is okay.
There was a problem hiding this comment.
There is something nice about having the testPlan.directoryin the URL. It feels more human-readable. Not requesting a change but more a point of discussion.
There was a problem hiding this comment.
I do feel like we still have enough leeway to change the URLs, I really doubt there are any bookmarks out there, but I appreciate you mentioning that!
For the URLs, my main motivation was to avoid using the whole git sha which made the URL feel unwieldy, but another option could be /test-review/checkbox-two-state/v23.08.23. This would require adding graphql support to query by versionString so it's a bit more work, but if you like it better maybe it's worth it?
That said, we generally use the TestPlanVersion ids elsewhere so this URL is pretty in keeping with our other URLs.
There was a problem hiding this comment.
That would definitely be the most human-readable URL but even just using the id with the test plan directory would add some info without adding a new query, like /test-review/checkbox-two-state/5. I tend to be biased towards putting as much human-readable information in the URL as possible but I definitely do not feel strongly about this. All of the options, including keeping as-is, work for me.
| target="_blank" | ||
| rel="noreferrer" | ||
| > | ||
| <a href={`/test-review/${testPlanVersion.id}`}> |
There was a problem hiding this comment.
Nice. I definitely agree with not opening the new tab.
| > | ||
| {title} | ||
| </a> | ||
| {isLast ? '' : ', '} |
There was a problem hiding this comment.
I understand that this is more visually compact by just making them into sibling <a>'s but the tradeoff is that the comma confuses VoiceOver (other ATs untested). I think it is worth the added scroll height to have a <ul> -> <li> -> <a> structure without commas here.
| {testPlanVersion.gitMessage} | ||
| </li> | ||
| </ul> | ||
| <h2>List of Tests</h2> |
There was a problem hiding this comment.
This component is pretty lengthy. I feel like the filtered tests could become their own component fairly easy. This would shorten this component and it seems likely that it could be reused elsewhere at some point since this section has a lot of standalone use value.
There was a problem hiding this comment.
I think this is a change that probably makes more sense to implement when this content is needed somewhere else, because I think for now the connective tissue needed like prop types might undercut some of the space saving benefits, and the component interface might be negatively affected by the need to support filtering out irrelevant ATs.
| @@ -57,8 +57,9 @@ const FilterButtons = ({ | |||
| }; | |||
|
|
|||
| FilterButtons.propTypes = { | |||
| filterLabel: PropTypes.string.isRequired, | |||
| filterAriaLabel: PropTypes.string, | |||
There was a problem hiding this comment.
Nice updates to this component!
There was a problem hiding this comment.
Looks good overall. I appreciate the attention to detail with a change that touches so many parts of the application. I compared this page to the previous page and there is a significant change in the text content present. There are also minor changes in the experience of navigating with a screenreader. I don't have the history with the project to know how proactive changes like this are received but thought it worth noting.
I am going to approve as all of my inline suggestions are optional.
There was a problem hiding this comment.
@alflennik looks good to me! Thanks for getting in this work and cleaning up so much. It's great that this page now really a part of the app!
commit fbf8eef Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Dec 14 14:46:48 2023 -0800 Clean up leftover import commit 15c594a Merge: 9c99b4f 56205a3 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Dec 14 14:29:54 2023 -0800 Merge branch 'main' into automation-mvp-merged commit 9c99b4f Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Dec 14 14:20:15 2023 -0800 Remove aria-at git commit cut off date commit 56205a3 Author: Howard Edwards <howarde.edwards@gmail.com> Date: Thu Dec 14 11:44:34 2023 -0500 Update tests (mock data and FilterButtons.test.jsx) and formatting of frontend components (#867) commit d63609d Author: Stalgia Grigg <stalgia@bocoup.com> Date: Wed Dec 13 16:16:26 2023 -0800 Fixes after merge commit 8ff6e6b Merge: 2e62661 8215cb9 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Wed Dec 13 15:03:10 2023 -0800 Merge branch 'main' into automation-mvp-rebase-attempt commit 8215cb9 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Tue Dec 12 07:59:44 2023 -0800 Trap backwards navigation from initial focus on header in focus-trapped modals (#851) * Add unit test for reverse navigation from initial focused header, FocusTrapper * Update unit tests for FocusTrapper, handle backwards navigation from initially focused header * Handle focus leaving non-focusable header element backwards in FocusTrapper commit 918acdd Author: Alexander Flenniken <alflennik@users.noreply.github.com> Date: Mon Dec 11 17:20:55 2023 -0500 Allow candidate test plans to advance with override (#832) * Allow candidate test plans to advance * Implement PR feedback commit 135e61b Author: Alexander Flenniken <alflennik@users.noreply.github.com> Date: Mon Dec 11 16:59:00 2023 -0500 Polish test review v2 page (#861) * Polish test review v2 page * Remove unused variable commit 234a339 Author: Alexander Flenniken <alflennik@users.noreply.github.com> Date: Thu Dec 7 14:57:42 2023 -0500 Update CONTRIBUTING.md (#859) commit 460b29c Author: alflennik <alflennik@users.noreply.github.com> Date: Thu Dec 7 13:26:17 2023 -0500 Update contributing.md commit 457caea Author: Howard Edwards <howarde.edwards@gmail.com> Date: Tue Dec 5 16:04:23 2023 -0500 Check for v2 testFormatVersion where applicable and hide link (#855) commit 6962cef Author: Howard Edwards <howarde.edwards@gmail.com> Date: Tue Dec 5 15:36:41 2023 -0500 Support v2 Test Format in import script and page changes (#840) * Update client/resources * Update import script to support v2 test format import * Update import script to include flattened commandsV2.json and add uniquely generated scenario and assertion IDs * Updating client and server files to support V2 test format * Remove console.log and add TODO * Include tokenized assertionStatement in TestPlanVersion.tests[x].assertions * Add retrieveCommands utility * Update candidate review and datamanagement page * Update Reports/queries to also include mayOptionalAssertionResults * Add support for AtMode and and referencing appropriate screen text in testsResolver * Remove console log * Fix edge case issue when saving test result and add priority standardizing utility * Add assertions checks for may assertions * Update getMetrics utils * Update test-import to account for single {at}-focused .collected file * Address file error * Fix tests * Update import branch * Fix edge case crash when viewing CandidateTestPlanRun * Update tests and prepare support for testing v2 format test plan versions * Update tests * Update workflow * Clarifying comment * Update runtest.yml to exclude v2 test format import * Update server/resolvers/TestPlanVersion/testsResolver.js Remove unnecessary `at.settings` check Co-authored-by: Stalgia Grigg <stalgia@bocoup.com> * Clearer differences between v1 and v2 test format tests being imported * Include typedef for RenderableContent * Update thrown error message when missing commands.json (v2) * Additional check for flattenObject * Fix bug found after rebase and update jsdoc on 'retrieveAttributes' * Updating Test Run page to support #743 * Update Test Run page instructions for v2 test format * Add react-html-parser; update InstructionsRenderer * Use testPlanVersion.metadata.testFormatVersion * Stop defaulting testFormatVersion to 1 * Stop using NumberedList * Update client/components/TestRenderer/utils.js Co-authored-by: Stalgia Grigg <stalgia@bocoup.com> * Define instructions variables * Prepend tests with 'Test {number}' on reports page * Address CG comments * Address PR feedback * Consistency * Passed/failed for assertion results in TestRenderer * Update test plan report conflict calculation to support pass/fail assertion results * Correct assertion legend, margin right for checkbox * Update inline snapshot for test queue * Remove failedReason * Update server/resolvers/TestResultOperations/saveTestResultCommon.js Co-authored-by: Howard Edwards <howarde.edwards@gmail.com> * Fix linting issue in saveTestResultCommon * Undo change to checkAssertionResult * Consistency * Do radio group and supporting docs * Revert "Do radio group and supporting docs" This reverts commit 579f545. * Revert aria-at import branch --------- Co-authored-by: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Co-authored-by: Stalgia Grigg <stalgia@bocoup.com> commit 703b01f Author: jugglinmike <mike@mikepennisi.com> Date: Tue Dec 5 12:25:21 2023 -0500 Do not render zeros in place of omitted content (#852) By relying on the "truthiness" of Number values to short-circuit boolean expressions, some rendering logic produced the text "0" instead of omitting any markup at all. Replace the expressions whose value could be coerced to booleans with expressions which directly evaluate to "fallback" values that do not produce markup (i.e. `false` or an empty array). commit 7783342 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Mon Dec 4 15:34:10 2023 -0800 Revert "Handle focus leaving non-focusable header element backwards in FocusTrapper" This reverts commit 57012cb. commit 57012cb Author: Stalgia Grigg <stalgia@bocoup.com> Date: Mon Dec 4 14:05:57 2023 -0800 Handle focus leaving non-focusable header element backwards in FocusTrapper commit 477abcb Author: Howard Edwards <howarde.edwards@gmail.com> Date: Wed Nov 29 10:10:10 2023 -0500 Remove additional css meant for now removed 'skipped' view (#848) commit dba7f40 Author: Alexander Flenniken <alflennik@users.noreply.github.com> Date: Tue Nov 28 14:37:24 2023 -0500 Implement Test Review in React (#846) * Implement client-side version of test-review * PR polish * Add additional specification features * Additional minor polish * PR feedback commit b98852b Author: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Date: Wed Nov 22 15:48:57 2023 -0500 No longer mark test plan reports with skipped tests as final (#833) * Hide mark as final button * No mark as final for skipped tests in graphql * Remove skipped tests from the UI * Add migration for unfinished tests * Simplify necessary files * Fix failing tests * Add code review fixes * Remove no-longer-necessary style class * Remove reference commit c29bea9 Merge: c101ecc c1bf56a Author: Stalgia Grigg <stalgia@bocoup.com> Date: Tue Nov 21 09:41:04 2023 -0800 Merge pull request #844 from w3c/fix-at-selection-dropdown Fix Data management page crash when Adding Test Plans to the Test Queue and Opening Assistive Technology select to pick any option commit c101ecc Merge: 878fe80 50cd6c8 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Tue Nov 21 09:40:43 2023 -0800 Merge pull request #837 from w3c/accessible-name-test-plan-report-status-dialog Fix aria-labeledby for BasicModal/BasicThemedModal commit 878fe80 Author: Paul Clue <67766160+Paul-Clue@users.noreply.github.com> Date: Mon Nov 20 15:08:58 2023 -0500 Fix empty version strings on Data Management and Test Queue pages (#839) * Add versionString to mutation * Remove comments * Fix similar issue happening on the Test Queue page when assigning and removing testers and also removing test results --------- Co-authored-by: Howard Edwards <howarde.edwards@gmail.com> commit a5f0e80 Author: Alexander Flenniken <alflennik@users.noreply.github.com> Date: Mon Nov 20 15:01:45 2023 -0500 Implement target date modal improvements (#835) * Implement target date modal improvements * Add esc support to target date modal commit c1bf56a Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Nov 16 11:29:56 2023 -0800 Update DataManagement test mock commit 9319e37 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Nov 16 11:16:37 2023 -0800 Remove browsers from query and state in TestQueue and DataManagement in favor of at.browsers commit 7dd31e1 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Thu Nov 16 11:07:46 2023 -0800 Fix bug that causes crash on DataManagement page ManageTestQueue AT dropdown commit 50cd6c8 Author: Stalgia Grigg <stalgia@bocoup.com> Date: Wed Nov 8 16:01:23 2023 -0800 Unit test for aria-labeledby on BasicModal commit c1e7f0f Author: Stalgia Grigg <stalgia@bocoup.com> Date: Wed Nov 8 15:42:55 2023 -0800 Fix aria-labeledby in both BasicModal and BasicThemedModal
Replaces the existing Test Review page, which is server-rendered with Handlebars and basically a direct port from a page on the ARIA-AT repo, with a client-rendered page in React, using the existing GraphQL endpoints to populate its data. Using existing React infrastructure allows us to resolve some limitations like being unable to put the normal nav bar on the page (see #788, which is fixed by this PR) and it also allows for a more compact implementation that's more similar to other pages which display test content.
Regarding #788, while bringing the page into React, its structure changed enough that it wouldn't make sense to implement the requested changes exactly as requested, but I do think all the requested information has been added in an equivalent form.