Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: Support minimum and exact AT Version requirements in Test Run's version selection modal #1131

Merged
merged 2 commits into from
Jun 20, 2024

Conversation

howard-e
Copy link
Contributor

This is related to discussions shared in #791. This PR makes the following changes to the <AtAndBrowserDetailsModal> component:

  1. If a minimum AT Version is set for a TestPlanReport, it prevents AT Versions released before the minimum specified AT Version to be selected on the Test Run page.
  2. If an exact AT Version is set for a TestPlanReport, it Includes a disclaimer message that it's expected that the tester is using the specified AT Version and prevents any AT Version selection. The disclaimer message is:

Results collected for this test plan require {atName} {atVersion}. By continuing, you confirm that your results are being recorded using the specified version of the Assistive Technology.

@howard-e howard-e mentioned this pull request Jun 17, 2024
@stalgiag stalgiag self-requested a review June 17, 2024 21:23
Copy link
Contributor

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes are clear and straightforward. I tested manually with VoiceOver by adding a test plan report with both minimum and exact requirements. With a minimum requirement, version options that do not meet the requirement are not shown in the dropdown. With an exact the warning is displayed and the version select is shown as a disabled text field with the required version. This reads/navigates well with VO.

Thanks for these changes!

Comment on lines +353 to +364
{exactAtVersion ? (
<>
<Form.Label>
Assistive Technology Version
</Form.Label>
<Form.Control
disabled
type="text"
value={exactAtVersion.name}
/>
</>
) : (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good UI choice!

);
return (
new Date(item.releasedAt) >=
earliestReleasedAt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point it might be good to break this out into a utility function so that we can unit test these comparisons to harden against any future changes to ATVersion. Not a requirement right now and I can easily add when doing the automation at version comparison work if preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll leave it up to a future PR because since there's a couple places where some date comparison is being done. Would rather some dedicated time be taken to pull those all together if creating a utility on that.

... and I can easily add when doing the automation at version comparison work if preferred

@stalgiag that's my preference right now, but great call out on this and definitely include me as a reviewer when doing so!

@howard-e howard-e merged commit d9374ca into trends Jun 20, 2024
2 checks passed
@howard-e howard-e deleted the atversion-requirements-testrun-modal branch June 20, 2024 18:03
howard-e added a commit that referenced this pull request Jun 20, 2024
This includes work to support #791 and #792.

Includes the following changes:
* #1055
* #1001
* #1065
* #1052 
* #1087
* #1098 
* #1092
* #1131
* #1124

---------

Co-authored-by: Howard Edwards <howarde.edwards@gmail.com>
Co-authored-by: Paul Clue <67766160+Paul-Clue@users.noreply.github.com>
Co-authored-by: alflennik <alflennik@users.noreply.github.com>
howard-e added a commit that referenced this pull request Jun 24, 2024
Includes the following changes:
* #1123, addresses #791 and #792 with:
  * #1055
  * #1001
  * #1065
  * #1052 
  * #1087
  * #1098 
  * #1092
  * #1131
  * #1124
* #1128, addresses #1100
* #1102, addresses #957
* #1132
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants