-
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
feat: Test Queue v2 #1124
feat: Test Queue v2 #1124
Conversation
April 8th 2024 Production Release
April 29, 2024 Production Release
…nner-in-support-table
title: 'Error Updating Assistive Technology Version' | ||
}); | ||
|
||
const loadedAts = useRef(false); |
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.
Is this used to prevent state updates on initial render? Just noting that this struck me as odd.
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.
Yep. This was originally moved from ManageTestQueue/index.js
and there was an attempt to remove it, but found that re-renders were being significantly caused with several actions so opted to just restore that for now.
setSelectedAtVersionId(value); | ||
}; | ||
|
||
const onOpenAtVersionModalClick = type => { |
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.
Curious why this pattern is used. Shorter independent functions for each one of these type
s could improve readability
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.
Agreed, also a copy over but no issue with addressing before landing.
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.
Unsure why this rename happened. My guess is that there is a reason for the metrics values to be re-evaluated but it's not immedidately obvious to me why. The calculatePercent
util was changed but not in any significant way but this also shouldn't hurt but something to be aware of.
title: 'Error Updating Assistive Technology Version' | ||
}); | ||
|
||
const loadedAts = useRef(false); |
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.
Yep. This was originally moved from ManageTestQueue/index.js
and there was an attempt to remove it, but found that re-renders were being significantly caused with several actions so opted to just restore that for now.
setSelectedAtVersionId(value); | ||
}; | ||
|
||
const onOpenAtVersionModalClick = type => { |
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.
Agreed, also a copy over but no issue with addressing before landing.
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.
Looking great! This is a major UI improvement. I left some comments inline. Most are just code quality and organization questions. I also recognize that much of this code wasn't written by others so not expecting you to speak to all of these things. I just wanted to document them for possible tech debt tasks.
I noticed some keyboard interaction issues with the "Add Test Plan to Test Queue" form. Two seem unique to this PR but otherwise happy to approve this as long as we make a list of tech debt follow-ups.
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.
A few questions inline in this file. I think this file could benefit from even more decomposition with managed child components. This can happen in a followup
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 noticed some issues with keyboard interaction on latest Chrome.
- Unable to open dropdowns with Enter, able to open with VO action key
- Unable to tab focus to unselected "Minimum Version" radio
- Unable to tab focus to "Select AT Version" or "Select a Browser" dropdown
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.
This item "Unable to open dropdowns with Enter, able to open with VO action key" is true on main so it doesn't need to be addressed with this PR. I would expect this element to interact in the same way as the APG's "Select only Combobox" but "Enter" has no effect.
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 noticed some issues with keyboard interaction on latest Chrome.
- Unable to open dropdowns with Enter, able to open with VO action key
Not opening with Enter
doesn't seem to be new when comparing against what's on main
so I'd like us to address this later if needed. But I also can't tell from a quick look on the select
spec if this is the expectation -- but in quickly testing a simple example across Safari, Chrome and Firefox, Enter
isn't handled in any, but Space
is. Is there a reason we should need to handle that?
- Unable to tab focus to unselected "Minimum Version" radio
Looks like that's the expected behavior for radiogroups
- Unable to tab focus to "Select AT Version" or "Select a Browser" dropdown
Hmmm! I suppose that decision was made to somewhat "improve" the experience since the AT being selected is what impacts which version options and browser options can actually be displayed. Do you think this way is more confusing than not?
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.
This item "Unable to open dropdowns with Enter, able to open with VO action key" is true on main so it doesn't need to be addressed with this PR. I would expect this element to interact in the same way as the APG's "Select only Combobox" but "Enter" has no effect.
Ah should have read ahead! Okay I understand your above point
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 like that's the expected behavior for radiogroups
Thanks for this! I looked at that example but for some reason, my brain didn't catch the tab vs arrow interaction working across both.
Hmmm! I suppose that decision was made to somewhat "improve" the experience since the AT being selected is what impacts which version options and browser options can actually be displayed. Do you think this way is more confusing than not?
This makes sense! I didn't fully understand the way it was supposed to be used but with that in mind it is an improvement. Thanks for clarifying.
Ah should have read ahead! Okay I understand your above point
Haha I should've went back and edited with a strikethrough. But yes agree that this should happen elsewhere.
I've tested a bunch at this point and feel comfortable saying that any issues I haven't uncovered yet can be fixed in a follow-up. I also feel fine with leaving any or all of the code structure improvements for follow-ups. I'll leave it to you which parts need to land now vs be noted as tech debt. I'll approve. Great work!
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.
Love it!
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.
It feels destructive to lose the commit history on these files in order to move them into the temporary TestQueue2
folder. It also makes it hard to see if there are any changes so I am assuming they remain unchanged.
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 did this one as well! I also considered the loss in history as well but didn't want to go through creating new branches in the existing components to support the changes needed. But in retrospect, perhaps that's much easier to handle than what I've included here.
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.
client/hooks/useForceUpdate.js
Outdated
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.
This feels like a bandaid for improper state management. There are definitely cases where things like this are necessary but it feels like a bad idea to build this directly into the 'show/hide' effects of basic components like our ConfirmationModal
s.
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.
Good point. I can certainly look more into why this way
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.
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.
Approving! See this comment for more.
…sting files to hold a temporary branch to support Test Queue v2 until v1 is removed
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.
This file is modified version of client/components/TestQueueCompletionStatusListItem/index.js
…label option text
This should be addressed by c04e71e |
…anReport as final with just a single tester assigned; instead provide alternate prompt asking confirmation on continuing
…luded elsewhere on reports page
Reverted this due to a visual problem -- will follow up in a subsequent change to not block on this |
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 <[email protected]> Co-authored-by: Paul Clue <[email protected]> Co-authored-by: alflennik <[email protected]>
This PR provides an updated version of the Test Queue as described at #791 (comment). It is available on the sandbox for live testing at
/test-queue
. The previous version is still available at/test-queue-old
.In advancing this work, found that were new design expectations of some of the previously used Test Queue components, found it easier to copy over and make the tweaks rather than bring destructive changes to the old test queue's pieces (and to also verify not breaking expected functionality). As such, there is additional cleanup that will come out of this change such as removing the old test queue or the copied over components, but I'd rather not inflate this PR further and instead follow up in a subsequent PR.