-
Notifications
You must be signed in to change notification settings - Fork 47
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
10461 Story: Public Trial Sessions Details Page #5552
10461 Story: Public Trial Sessions Details Page #5552
Conversation
…b for the frontend
… existing gotoTrialSessionDetailSequence
… TrialSessionDetailHeader to avoid using cerebral
…tioner and respondent counsel information to show
…et counsels displayed
…he internal trial sessions process; add isSealed to CalendaredCase; the NonMobile UI is essentially functional
…how Sealed for public case title if sealed
…he rename now triggers 'new' type errors in our CI check for old, renamed files
@@ -165,7 +165,7 @@ describe('Dismiss NOTT reminder on calendared trial session within 30-35 day ran | |||
await cerebralTest.runSequence('dismissThirtyDayTrialAlertSequence'); | |||
|
|||
expect(cerebralTest.getState('currentPage')).toEqual( | |||
'TrialSessionDetail', | |||
'TrialSessionDetails', |
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.
Some places had Detail
(singular), others had Details
(plural). I made them all Details
.
const { output } = await runAction< | ||
{ trialSession: PublicTrialSessionDetails }, | ||
PublicClientState | ||
// @ts-ignore |
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.
Typing for this was a bear. I decided it wasn't worth it. If anyone knows of an easy fix, I'm happy to implement. My workaround fixed the type error but just ended up making the codebase slightly worse.
import { ClientPublicApplicationContext } from '@web-client/applicationContextPublic'; | ||
import { PublicTrialSessionDetails } from '@web-api/business/useCases/trialSessions/getPublicTrialSessionDetailsInteractor'; | ||
|
||
export const getPublicTrialSessionDetailsAction = async ({ |
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 you compare with getTrialSessionDetailsAction.ts
, you'll notice that we are making one less persistence call here. This is because the interactor for getTrialSessionDetailsAction.ts
does not get swing session details, so the action has to make a separate call for them. For public trial session details, I put that swing session fetching logic on the interactor itself.
@@ -1,3 +1,4 @@ | |||
import { PublicClientState } from '@web-client/presenter/state-public'; |
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.
Here and below, I'm just fixing some type errors among the public tests.
@@ -0,0 +1,42 @@ | |||
import { ConsolidatedCaseIcon } from '@web-client/ustc-ui/Icon/ConsolidatedCaseIcon'; |
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.
Rather than have this icon code duplicated in several spots across the trial session details pages, I pulled it out here. Like sorting, I have seen us handle case/docket icons in different ways, and I hope this is a step towards avoiding these distinctions without difference.
return ( | ||
<div | ||
className="multi-filing-type-icon" | ||
style={{ display: 'flex', flexWrap: 'nowrap', gap: '1rem' }} // USWDS overwrites these styles at certain breakpoints even with !important |
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 don't know how else to get around this USWDS style issue.
> | ||
<span | ||
className={ | ||
formattedCase.isSealed ? 'visibility-visible' : 'visibility-hidden' |
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 keep the icon present but hidden/inaccessible so that spacing is always consistent.
@@ -20,7 +20,9 @@ export const ConsolidatedCaseIcon = ({ | |||
icon="copy" | |||
/> | |||
{showLeadCaseIcon && ( | |||
<span className="fa-inverse lead-case-icon-text">L</span> | |||
<span aria-hidden={true} className="fa-inverse lead-case-icon-text"> |
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.
Updating aria-hidden so that screen readers don't say "L."
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 catch!
@@ -41,10 +45,12 @@ export const AppComponentPublic = connect( | |||
currentPage: state.currentPage, | |||
}, | |||
function AppComponentPublic({ currentPage }) { | |||
const focusMain = e => { | |||
e && e.preventDefault(); | |||
const focusMain = (e?: React.MouseEvent) => { |
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.
Fixing some type errors
/> | ||
); | ||
})} | ||
<tbody> |
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.
Pulling tbody
outside of the map. Thanks to Jim L. for catching this.
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.
Thanks!
@@ -2,6 +2,7 @@ import { Icon } from '@web-client/ustc-ui/Icon/Icon'; | |||
import React from 'react'; | |||
|
|||
// This might be useful for more than just practitioners | |||
// It is similar to CaseIcons, but without indentation |
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.
Potential refactor fodder.
{publicTrialSessionsHelper.trialSessionsCount} | ||
</div> | ||
<div className="padding-1"></div> | ||
<div className="overflow-x-auto"> |
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.
Added scroll as needed
<div className="text-right"> | ||
<span className="text-semibold">Count: </span> | ||
{allCases.length} | ||
</div> | ||
<div className="padding-1"></div> | ||
<div className="overflow-x-auto overflow-y-hidden"> |
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 updated all of these similar files to style and position the Count indicator in the same way, and added scrolling as needed. This required wrapping much of the component in a div, which registers a deceptively large git diff. The only other changes are using the new "Icons for consolidated and/or sealed cases"
text for aria-label.
@@ -75,6 +77,7 @@ export const TrialSessionDetail = connect( | |||
<TrialSessionInformation /> | |||
|
|||
{!formattedTrialSessionDetails.isCalendared && ( | |||
/* @ts-ignore: Tabs needs to be refactored to avoid type errors */ |
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.
Renaming file caused these type errors to register as "new" and thus fail the CI Typescript checks.
@@ -65,3 +65,111 @@ describe('sortByDocketNumber', () => { | |||
]); | |||
}); | |||
}); | |||
|
|||
describe('sortByDocketNumberAndGroupConsolidatedCases', () => { | |||
it('should return the cases sorted properly, with a consolidated case appearing after its lead cases', () => { |
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.
cases -> case, which I will correct either in a commit addressing other feedback or else in a follow-up PR.
]); | ||
}); | ||
|
||
it('should return the cases sorted properly, when cases contains a consolidated grouping and the lead case is missing', () => { |
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.
The next two tests come almost directly from shared/src/business/utilities/trialSession/getFormattedTrialSessionDetails.compareCasesByDocketNumber.test.ts
, which test the function that my new sort function is meant to replicate and eventually replace. (I made one small change to put the two member cases out of order in the second test, which makes the test slightly more robust.)
}); | ||
|
||
it('should be free of a11y issues', () => { | ||
cy.visit('/trial-session-detail/959c4338-0fac-42eb-b0eb-d53b8d0195cc'); |
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.
Does this trial session id come from seed data? Is this allowed for local-only
tests?
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 does come from the seed data, and yes. (It is basically just a copy of this:
describe('Trial session details', () => { | |
it('should be free of a11y issues', () => { | |
loginAsColvin(); | |
cy.visit('/trial-session-detail/959c4338-0fac-42eb-b0eb-d53b8d0195cc'); | |
cy.contains('Session Information').should('exist'); | |
checkA11y(); | |
}); | |
}); |
// No need to see docket entries | ||
const caseWithEmptyDocketEntries = { | ||
...aCase, | ||
docketEntries: [], | ||
}; |
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.
Excellent idea!
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.
Thanks to @ttlenard for catching it!
@@ -20,7 +20,9 @@ export const ConsolidatedCaseIcon = ({ | |||
icon="copy" | |||
/> | |||
{showLeadCaseIcon && ( | |||
<span className="fa-inverse lead-case-icon-text">L</span> | |||
<span aria-hidden={true} className="fa-inverse lead-case-icon-text"> |
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 catch!
/> | ||
); | ||
})} | ||
<tbody> |
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.
Thanks!
It is probably best to merge 10460 into staging first and then this as soon as we can after.Done!This way we avoid reviewing duplicate code, and I can address any feedback on 10460 and incorporate it into my PR if need be.
Ticket here. Related ticket is the public Trial Sessions page (10460), with the PR here.
This PR implements the public Trial Sessions Details page. It also adds sealed icons to cases listed in the internal Trial Sessions Details page.
Notes: