Skip to content

LG-7771 Add correct screenreader tag for incomplete steps #7613

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.

Sign up for GitHub

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

Merged
soniaconnolly merged 1 commit into from
Jan 11, 2023
Merged

LG-7771 Add correct screenreader tag for incomplete steps #7613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/javascript/packages/step-indicator/step-indicator-step.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ describe('StepIndicatorStep', () => {
);

const title = getByText('Step');
const status = getByText('step_indicator.status.current');
const status = getByText('step_indicator.status.not_complete');
Copy link
Contributor

@aduth aduth Jan 11, 2023

Choose a reason for hiding this comment

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

Can we add a test case for the "current step" context?

Copy link
Contributor Author

@soniaconnolly soniaconnolly Jan 11, 2023

Choose a reason for hiding this comment

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

Is there something you want covered in addition to lines 5-20 in this file?

Copy link
Contributor

@aduth aduth Jan 11, 2023

Choose a reason for hiding this comment

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

Is there something you want covered in addition to lines 5-20 in this file?

Oh, good point! I totally missed we had test coverage for StepStatus.CURRENT already. I suppose it could be nice to have all of them covered (PENDING seems to be missing), but that's not in the scope of your work, so I wouldn't consider it a blocker.

Copy link
Contributor Author

@soniaconnolly soniaconnolly Jan 11, 2023

Choose a reason for hiding this comment

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

We were wondering about PENDING, and if it was really used. I'll go ahead and merge this, but there might be additional cleanup. Do you know what PENDING is for?

Copy link
Contributor

@aduth aduth Jan 11, 2023

Choose a reason for hiding this comment

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

It was originally used for marking the GPO letter (verify by mail) step as pending, but I seem to recall there may have been some recent revisions to the flow where we no longer show the pending state. Let me dig that up, since it would be nice if we could drop the extra logic (I had a hard time re-wrapping my head around the differences between "incomplete", "current", and "pending").

Copy link
Contributor

@aduth aduth Jan 11, 2023

Choose a reason for hiding this comment

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

It looks like the pending status was meant to have been removed as part of #6860. I can follow-up to clean up the lingering references, since I'm to blame for not having done it in the first place 😅

const step = title.closest('.step-indicator__step')!;

expect(title).to.be.ok();
Expand Down
5 changes: 4 additions & 1 deletion app/javascript/packages/step-indicator/step-indicator-step.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export interface StepIndicatorStepProps {
}

function StepIndicatorStep({ title, status }: StepIndicatorStepProps) {
const { CURRENT, COMPLETE, PENDING } = StepStatus;
const { CURRENT, COMPLETE, PENDING, INCOMPLETE } = StepStatus;

const classes = [
'step-indicator__step',
Expand All @@ -35,6 +35,9 @@ function StepIndicatorStep({ title, status }: StepIndicatorStepProps) {
case COMPLETE:
statusText = t('step_indicator.status.complete');
break;
case INCOMPLETE:
statusText = t('step_indicator.status.not_complete');
break;
case PENDING:
statusText = t('step_indicator.status.pending');
break;
Expand Down