-
Notifications
You must be signed in to change notification settings - Fork 4
Feat: Adding Breadcrumbs to the payroll flow component #680
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
Conversation
@@ -0,0 +1,58 @@ | |||
@use '../../../../styles/Helpers' as *; |
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.
WIP: this is auto-generated style - will adjust in the follow up
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbs.tsx
Outdated
Show resolved
Hide resolved
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbs.tsx
Outdated
Show resolved
Hide resolved
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbs.tsx
Outdated
Show resolved
Hide resolved
onEvent(componentEvents.RUN_PAYROLL_EMPLOYEE_EDIT, { employeeId: employee.uuid }) | ||
onEvent(componentEvents.RUN_PAYROLL_EMPLOYEE_EDIT, { | ||
employeeId: employee.uuid, | ||
firstName: employee.firstName, |
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.
To display employee name in the breadcrumb
[componentEvents.RUN_PAYROLL_EDIT]: undefined | ||
} | ||
|
||
export const payrollFlowBreadcrumbsNodes: BreadcrumbNodes = { |
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 is the structure that establishes relationships between breadcrumbs for the flow
key: 'landing', | ||
label: 'breadcrumbs.landing', | ||
namespace: 'Payroll.Flow', | ||
onNavigate: ((ctx: PayrollFlowContextInterface) => ({ |
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.
Basically custom reducer for a specific breadcrumb
}) | ||
} | ||
|
||
const breadcrumbNavigateTransition = (targetState: string) => |
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 is a factory that creates a specific transition for allowed breadcrumb - for now, each state needs to define all possible breadcrumb transitions - this can be automated, but we might prefer explicit approach in the spirit of FSMs
transition( | ||
componentEvents.BREADCRUMB_NAVIGATE, | ||
targetState, | ||
guard( |
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.
guard ensures that transition to a target state happens if relative breadcrumb is clicked
@@ -0,0 +1,145 @@ | |||
import 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.
This is breadcrumb node to trail resolution plus hydration of breadcrumbs with updated variables/translations
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.
Pull Request Overview
This PR introduces a breadcrumb navigation system for the Payroll Flow, replacing the step-based progress bar with clickable breadcrumbs. The implementation includes helper functions for building and managing breadcrumb trails, a new ProgressBreadcrumbs
component, and updates to the payroll state machine to support breadcrumb-based navigation.
Key Changes:
- Added breadcrumb navigation system with helper functions for building trails and resolving template variables
- Created
ProgressBreadcrumbs
component with full test coverage and Storybook stories - Updated payroll flow to use breadcrumbs instead of step numbers for navigation
- Removed back button functionality from
PayrollOverview
in favor of breadcrumb navigation
Reviewed Changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/helpers/breadcrumbHelpers.ts | Core helper functions for building breadcrumb trails and resolving template variables |
src/helpers/breadcrumbHelpers.test.ts | Comprehensive test suite for breadcrumb helper functions |
src/components/Common/UI/ProgressBreadcrumbs/* | New breadcrumbs component with types, styles, tests, and stories |
src/components/Payroll/PayrollFlow/payrollStateMachine.ts | Updated state machine with breadcrumb nodes and navigation transitions |
src/components/Payroll/PayrollFlow/PayrollFlow.tsx | Integrated breadcrumb building and i18n support |
src/components/Flow/Flow.tsx | Added conditional rendering for breadcrumbs vs progress bar |
src/components/Payroll/PayrollOverview/* | Removed back button and showBackButton prop |
src/i18n/en/Payroll.Flow.json | Translation keys for breadcrumb labels |
src/shared/constants.ts | Added BREADCRUMB_NAVIGATE event constant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
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.
Left some feedback primarily around the separation of the Breadcrumbs UI and our application logic/utilities. Essentially we want the component in UI to only deal with the presentation and have the simplest API possible. We'll want to remove the translation and event concerns from that component.
One suggestion I have that might simplify this would be to create an additional Breadcrumb component in the common
directory that will utilize the presentation component from UI and can be provided with our app specific configuration. That'll allow us to centralize our app complexity for managing breadcrumbs while keeping it simple for the partners to wire up adapters to the UI portions
Let's set up time to pair and we can chat more about it
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbs.tsx
Outdated
Show resolved
Hide resolved
/** | ||
* Event handler for breadcrumb navigation | ||
*/ | ||
onNavigate?: (context: unknown) => unknown |
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.
My rec here would be to
- Remove
onNavigate
from the Breadcrumb and move it to the ProgressBreadcrumb props below - Update
onNavigate
to have the uniquekey
orid
above passed as the argument - Remove
onEvent
from below
(Note: I'm flexible on the onNavigate
being here or in the parent, placing it in the parent would keep it a little more aligned with Tabs where you have a single callback function for the component when it is selected that is called with the id of the item as an argument)
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbsTypes.ts
Outdated
Show resolved
Hide resolved
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbsTypes.ts
Outdated
Show resolved
Hide resolved
src/components/Common/UI/ProgressBreadcrumbs/ProgressBreadcrumbs.tsx
Outdated
Show resolved
Hide resolved
<button | ||
type="button" | ||
className={styles.link} | ||
onClick={() => { | ||
handleBreadcrumbClick(breadcrumb) | ||
}} | ||
> |
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.
@jeffredodd should this have a role of link set even though it is a button?
{progressBarType === 'breadcrumbs' && ( | ||
<Components.ProgressBreadcrumbs | ||
breadcrumbs={currentBreadcrumb ? (breadcrumbs[currentBreadcrumb] ?? []) : []} | ||
cta={current.context.progressBarCta} |
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.
Ex. my rec here would be to break out the CTA and just align that here with a flex layout instead of having breadcrumbs needing to worry about the CTA
Breadcrumb, | ||
BreadcrumbNodes, | ||
BreadcrumbNode, | ||
BreadcrumbTrail, |
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 more at the usage, i think we should implement a Common
component that can expose these interfaces which wraps the Components.Breadcrumbs
from UI. That'll allow us to implement the complexity seen here that is specific to our codebase, while allowing the UI portion of the breadcrumb to be simplified by consumers
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 nits, nothing huge
}, | ||
Breadcrumbs: ({ breadcrumbs, currentBreadcrumbId, ariaLabel, onClick }: BreadcrumbsProps) => { | ||
return ( | ||
<div className="progress-breadcrumbs"> |
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.
Would you mind doing this with a nav like wai aria shows? Should be a simple change https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/
|
||
&:focus-visible { | ||
outline: var(--g-focusRingWidth) solid var(--g-focusRingColor); | ||
outline-offset: 2px; |
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.
too bad we don't have --g-focusRingOffset
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.
LGTM! Thanks for updating!
Screen.Recording.2025-10-16.at.3.25.26.PM.mov