-
Notifications
You must be signed in to change notification settings - Fork 117
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
ui v2: add stepper component #691
Conversation
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.
nice!! 🎉
|
||
const StepConnector = styled(MuiStepConnector)((props: { completed?: boolean }) => ({ | ||
".MuiStepConnector-line": { | ||
height: "5px", |
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 think the height on figma is 7px
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.
oh it depends on which bar you read the height from. the bar example in the last row specifies 5px.
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 think it is supposed to be 7px, left a comment on figma for clarification though
|
||
const Stepper = ({ activeStep, children }: StepperProps) => ( | ||
<div> | ||
<MuiStepper style={{ padding: "0" }} activeStep={activeStep + 1} connector={<StepConnector />}> |
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 push the inline styles up to the parent div and use selectors?
</MuiStepper> | ||
<Grid style={{ padding: "16px 0" }} container justify="space-between"> | ||
{React.Children.map(children, (step: any, idx: number) => ( | ||
<StepLabel item data-active={idx === activeStep}> |
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.
thinking about it more what about using a class name instead of data-.
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 kind of like using props here since it makes it clear what we are specifying compared to what Material is injecting
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.
though data won't cascade for nested selectors, i think in general we should standardize on classes. we can use a prefix to distinguish from Mui if needed.
note: (comment after merge, not a blocker)
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.
👍 going to make a cleanup task and add some of the standardizing bits we have talked about to it
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, discretionary nits
Description
Add a stepper component. I've improvised with an error icon for now. We can update once we get finalized designs.
Once we settle on colors we can hoist these up into the palette
Testing Performed
manual via story