-
Notifications
You must be signed in to change notification settings - Fork 64
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
feat(component): add Stepper component #503
Conversation
@chanceaclark Can you take a look at this when you have some time please? @Spawbreaker can you merge master into this branch so it's not out of date? Thanks! |
Hey @Spawbreaker @bc-prestonhuth, sorry about the wait. The last few weeks have been quite busy/blizzardous. I'll get around to reviewing this in the next day or so. |
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.
Hey @Spawbreaker, I have a ton of comments regarding internal structure of this component and how to simplify it. To make this a little easier to communicate from my end, I created another branch to show the changes I would like to make:
Spawbreaker/big-design@stepper...chanceaclark:stepper-updates
I will also inline comments in regards to those changes for what I changed. Keep in mind if I comment on something, it may be the same for a bunch of places.
Otherwise it's looking really good! Love some of the CSS stuff you did (though I change some stuff 😉 )
active?: boolean; | ||
} | ||
|
||
export const Step: React.FC<StepProps> = ({ number, text, state, isLast }) => { |
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.
Move this component into its own file: .../src/components/Stepper/Step/Step.tsx
const [isCurrent, setIsCurrent] = useState(state === 'current'); | ||
const [isDone, setIsDone] = useState(state === 'done'); | ||
useEffect(() => { | ||
setIsCurrent(state === 'current'); | ||
setIsDone(state === 'done'); | ||
}, [state]); |
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 would say that this Stepper
component is not a stateful component. We should avoid having internal state where it's not needed. This is more of a component where you pass state into it and you can derive values from it. See what I mean here.
export const StyledStepper = styled.div` | ||
display: grid; |
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.
We should be extending from our built in component rather than defining our own display properties, where possible.
export const StyledStepper = styled.div` | |
display: grid; | |
export const StyledStepper = styled(Grid)` |
That way you can use built in helpers:
<StyledStepper marginBottom="xLarge" />
export const StyledStepper = styled.div` | ||
display: grid; | ||
margin-bottom: 1.5rem; | ||
text-align: left; |
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.
Not needed.
text-align: left; |
export interface LightProps { | ||
active?: boolean; | ||
} | ||
export interface DashProps { | ||
active?: boolean; | ||
} | ||
export interface TextProps { | ||
active?: boolean; | ||
} |
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 they have the same props, might want to name the interface something a bit more generic to reuse. In my branch I didn't use these, in favor of using dynamic props by extending our utility components, like so.
}, [state]); | ||
|
||
return ( | ||
<StyledStep> |
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.
Couple of things with this component:
- I think using a
Flex
is better that using a grid. According to the designs, each step should be the same length as the others and using that gridminmax(6rem, 14.25rem)
logic will cause them to collapse non-uniformly on smaller screen sizes. - This is a big miss – we need to add in a11y props. I added them here. Here is also the https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_progressbar_role
<StyledStep> | ||
<StyledLight active={isCurrent || isDone}> | ||
{isDone ? ( | ||
<CheckIcon color="white" size="large" style={{ padding: '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.
We should almost never use the style
attribute.
export const StyledStep = styled.div` | ||
display: grid; | ||
${({ theme }) => theme.breakpoints.tablet} { | ||
grid-template-columns: 1.5rem 1fr; |
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.
When working with spacing value, use the ${({ theme }) => theme.spacing.<spacing value>}
instead of hard coding rem
units. If any values are bigger than our predefined spacing values, consider using ${({ theme }) => theme.spacing.remCalc(96)} // 6rem
.
const steps = ['Step 1', 'Step 2', 'Step 3']; | ||
const currentStep = 0; | ||
|
||
return <Stepper steps={steps} currentStep={currentStep} />; |
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.
Let's add some interaction to this example to show the different states. Suggestion:
return <Stepper steps={steps} currentStep={currentStep} />; | |
return ( | |
<> | |
<Stepper steps={steps} currentStep={currentStep} /> | |
<Button onClick={() => setCurrentStep(currentStep + 1)} disabled={currentStep === steps.length - 1}> | |
Next | |
</Button> | |
<Button onClick={() => setCurrentStep(0)} disabled={currentStep === 0}> | |
Reset | |
</Button> | |
</> | |
); |
const { container } = render(<Stepper steps={steps} currentStep={1} />); | ||
expect(container.firstChild).toMatchSnapshot(); | ||
}); | ||
|
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.
Let's add some more tests to make sure passed in state works as expected (hint: no snapshot testing for these).
currentStep: number; | ||
} | ||
|
||
export const Stepper: React.FC<StepperProps> = memo(({ className, style, steps, currentStep, ...props }) => ( |
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.
Instead of providing defaultProps below, we may want to inline them here. Maybe even exclude steps
having a default prop. Also, we may want to check if the provided steps
array has length to ensure we don't render anything if an empty array is provided. The main reasoning for all of this is to guard rendering in non-Typescript projects + empty arrays.
export const Stepper: React.FC<StepperProps> = memo(({ className, style, steps, currentStep, ...props }) => ( | |
export const Stepper: React.FC<StepperProps> = memo(({ className, style, steps, currentStep = 0, ...props }) => steps.length ? ( | |
// make sure to finish the ternary |
}, | ||
{ | ||
name: 'currentStep', | ||
types: 'number', |
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 should also be required.
<H0>Stepper</H0> | ||
|
||
<Text> | ||
The <Code primary>Stepper</Code> component is used to display a set number of steps. Useful for guiding an user |
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 <Code primary>Stepper</Code> component is used to display a set number of steps. Useful for guiding an user | |
The <Code primary>Stepper</Code> component is used to display a set number of steps. Useful for guiding a user |
<CodePreview> | ||
{/* jsx-to-string:start */} | ||
{function Example() { | ||
const steps = ['Step 1', 'Step 2', 'Step 3']; |
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 make these steps a little more "real world"? e.g. "Login", "Import products", etc...
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.
Looks good to me!
What
Adds the Stepper component, a new component to track progress on multi-step tasks.
Why
Feature request for a Stepper component