Skip to content

LG-6307: Remove pending steps from step indicator#6860

Merged
aduth merged 5 commits intomainfrom
aduth-lg-6307-step-indicator-pending
Sep 2, 2022
Merged

LG-6307: Remove pending steps from step indicator#6860
aduth merged 5 commits intomainfrom
aduth-lg-6307-step-indicator-pending

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Aug 29, 2022

Related: #6846

Why: As a user, I want to see a step indicator at the top of the page that shows me moving forward in the flow, so that it's easy to understand that I'm making progress and what I still need to do.

Screenshots:

Step Before After
GPO Opt-in Screen Shot 2022-08-29 at 10 44 23 AM Screen Shot 2022-08-29 at 10 44 23 AM
Password confirm Screen Shot 2022-08-29 at 10 44 38 AM Screen Shot 2022-08-29 at 10 42 28 AM
Personal key Screen Shot 2022-08-29 at 10 44 45 AM Screen Shot 2022-08-29 at 10 42 35 AM
Come back soon Screen Shot 2022-08-29 at 10 44 52 AM Screen Shot 2022-08-29 at 10 42 42 AM
GPO Verification Screen Shot 2022-08-29 at 10 44 58 AM Screen Shot 2022-08-29 at 10 42 50 AM

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

is this the line the eslint-plugin change was for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is this the line the eslint-plugin change was for?

Yep, sorry, I should have highlighted that connection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the previous rule enforced, you'd have to do something like...

let { steps } = FLOW_STEPS_PATH[path];
const { mapping } = FLOW_STEPS_PATH[path];

Not only is it kinda garish, I think the general pattern could be problematic in cases such as...

let { foo } = doSomeExpensiveOrMutativeTask();
const { bar } = doSomeExpensiveOrMutativeTask();

It's not always straight-forward to split const/let assignments for a destructure, and since the idea with preferring const is largely around developer experience and communicating intent, the readability benefits of allowing a single destructured "let" assignment outweigh that of enforcing const.
**Why**: As a user, I want to see a step indicator at the top of the page that shows me moving forward in the flow, so that it's easy to understand that I'm making progress and what I still need to do.

changelog: Improvements, Identity Verification, Remove pending steps from step indicator
It's now unused. YAGNI!
@aduth aduth force-pushed the aduth-lg-6307-step-indicator-pending branch from 396506c to fba1bf0 Compare September 1, 2022 17:42
Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Reviewed in sandbox. Looks great to me too!

@aduth aduth merged commit 93a5f05 into main Sep 2, 2022
@aduth aduth deleted the aduth-lg-6307-step-indicator-pending branch September 2, 2022 14:17
@zachmargolis zachmargolis mentioned this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants