-
Notifications
You must be signed in to change notification settings - Fork 34
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
Stepper component #2465
base: main
Are you sure you want to change the base?
Stepper component #2465
Conversation
? odysseyDesignTokens.HueBlue600 | ||
: odysseyDesignTokens.HueNeutral600 | ||
}`, | ||
background: completed |
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.
Nit: Can we be more explicit and use backgroundColor
here.
Also, if possible, can we do a more conditional approach to applying these? Rather than a nested ternary. This is less of a 'nit' as I think we'd like to generally move this direction with these types of styles
...(completed && (...
...(active && (...
: active | ||
? odysseyDesignTokens.HueBlue600 | ||
: "transparent", | ||
transition: `all ${odysseyDesignTokens.TransitionDurationMain}`, |
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'd recommend against using 'all' here and be more explicit about what exactly is transitioning. There are performance concerns with all
and, you get the added bonus of being able to tell at a glance what is being transitioned
transition: `all ${odysseyDesignTokens.TransitionDurationMain}`, | ||
|
||
".MuiStep-root:hover &": | ||
!active && !completed && nonLinear |
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.
Same comment about the conditional pattern, rather than a ternary here
completed || active | ||
? odysseyDesignTokens.HueNeutralWhite | ||
: odysseyDesignTokens.HueNeutral700, | ||
border: `1px solid ${ |
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.
nit: Can we break this into 2 declarations.
border: '1px solid',
And then conditionally add the borderColor
? { border: `1px solid ${odysseyDesignTokens.HueNeutral900}` } | ||
: undefined, | ||
|
||
"& svg": { |
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.
Are we targeting the child svg
here? If so, we can lose the &
.
|
||
"& svg": { | ||
width: | ||
variant === "numeric" |
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.
Same comment about the conditional pattern
? odysseyDesignTokens.Spacing4 | ||
: odysseyDesignTokens.Spacing3, | ||
height: | ||
variant === "numeric" |
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.
Same comment about the conditional pattern
/** | ||
* Aria label for the stepper container, Falls back to "Progress steps" | ||
*/ | ||
ariaLabel?: 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.
Can we define this as ariaLabel?: HtmlProps["ariaLabel"];
. You can add additional comments by doing this:
/**
*
* Aria label for the stepper container, Falls back to "Progress steps"
*/
Notice the extra *
above
/** | ||
* Callback fired when back button is clicked | ||
*/ | ||
onBack: () => void; |
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.
What do we send to the consumer onBack
or onNext
? Do they need a name or number of the step they are coming from? I would assume they would but, not sure
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 callbacks aren't currently sending any context, which definitely seems limiting. I'm going to update to add current and target step indexes.
display: "grid", | ||
gridTemplateColumns: "1fr auto 1fr", | ||
alignItems: "center", | ||
marginTop: odysseyDesignTokens.Spacing3, |
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.
marginTop: odysseyDesignTokens.Spacing3, | |
marginBlockStart: odysseyDesignTokens.Spacing3, |
status === "current" ? odysseyDesignTokens.HueNeutral500 : "transparent", | ||
margin: "0 2px", | ||
cursor: isClickable ? "pointer" : "default", | ||
"&:hover": isClickable |
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'd move all these isClickable
styles into it's own block like
...(isClickable && ({
...all your clickable styles
border: "1px solid", | ||
borderRadius: "50%", | ||
borderColor: | ||
status === "current" |
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.
Same as below. Can move these into their own blocks like
...(status === 'current') && ({...
(excludedProps: string[]) => (prop: string) => | ||
!excludedProps.includes(prop); | ||
|
||
export const shouldForwardStepProps = createShouldForwardProp([ |
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'm all for abstraction but, I think we could get away with just using the base createShouldForwardProp
inline when creating styled components. Although, I'm unsure about the name in that case. Maybe like... filterExcludedProps
?
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.
yeah that makes sense to me, it could be more explicit that way
c147916
to
04df1a9
Compare
DES-6996
Summary
Adds Stepper component
Component checklist
Code structure
Accessibility implementation
Internationalization (i18n) implementation
Documentation
Finalize code
QA