feat: Add UI for Managing Required Reports for Phases #1205
Conversation
Co-authored-by: Paul-Clue <[email protected]>
There was a problem hiding this comment.
@stalgiag thanks for restoring this work! I've left some inline comments, but also acknowledging that this wasn't originally created by you (and it's been quite some time). So if you'd like to discuss anything further first, we certainly can
server/graphql-schema.js
Outdated
| enum RequiredReportPhase { | ||
| IS_CANDIDATE | ||
| IS_RECOMMENDED | ||
| } |
There was a problem hiding this comment.
Maybe this could reuse the TestPlanVersionPhase enum instead of introducing a new enum to reduce noise?
There was a problem hiding this comment.
Yeah I wasn't sure the logic behind the IS_X pattern but since the previous PR was accepted for that I decided to keep it. Can modify.
There was a problem hiding this comment.
Addressed in 1f7db13
| </PhasePill> | ||
| </td> | ||
| <td>{at.name}</td> | ||
| <td>{browser.name}</td> |
There was a problem hiding this comment.
browser.name isn't coming through for the rows on the Data Management page (which also affects the edit and delete functions on that page)
There was a problem hiding this comment.
Oh interesting. I wasn't testing on the Data Management page but realizing now that this disclosure wasn't intended to exist on that page and a boolean prop in ManageTestQueue was being used to exclude it previously. I'll do that.
| return ( | ||
| <> | ||
| <DisclosureContainer data-testid="manage-required-reports-disclosure"> | ||
| <span>Add required reports for a specific AT and Browser pair</span> |
There was a problem hiding this comment.
| <span>Add required reports for a specific AT and Browser pair</span> | |
| <span>Update which assistive technology and browser combinations require reports for the Candidate and Recommended phases</span> |
May need some workshopping but this is for a bit more clarity
| <Dropdown.Menu className="drop-down-div" as={CustomMenu}> | ||
| {['Candidate', 'Recommended'].map(phase => ( | ||
| <Dropdown.Item | ||
| key={phase} | ||
| className="phase-option" | ||
| onClick={() => handleInputChange('phase', phase)} | ||
| > | ||
| {phase} | ||
| </Dropdown.Item> | ||
| ))} | ||
| </Dropdown.Menu> |
There was a problem hiding this comment.
This custom menu might be a bit much for just this as I'm not seeing a major advantage to highlighting the selected option's color. It also feels a bit out of place given this and the connected disclosures' options. This feels like a case where simplicity may be better, with a native <select>.
There was a problem hiding this comment.
This is another case where I agree but didn't want to deviate from the pattern that was previously used. I knew that you and Alex were pairing on the code so I didn't want to be too presumptuous about what was discardable. I assume this custom menu was being used to replicate the original mockups as closely as possible.
There was a problem hiding this comment.
Addressed in 43df566
stalgiag
changed the title
Co-authored-by: howard-e <[email protected]>
…of CustomMenu and CustomToggle
|
I believe that all of the feedback was addressed. The PR is much better for it. Thanks for that! |
This PR is a migration of the work completed by @Paul-Clue in #774. Thanks Paul!
There are significant differences between that PR and this one due to changing practices in the repo and structural shifts since that PR was opened. I chose to squash all of the functional work into a single PR to retain attribution where it is due.
see #683