Skip to content
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] Difficult to compose without learning implementation details #21326

Closed
Floriferous opened this issue Jun 5, 2020 · 9 comments · Fixed by #21613
Closed

[Stepper] Difficult to compose without learning implementation details #21326

Floriferous opened this issue Jun 5, 2020 · 9 comments · Fixed by #21613
Labels
component: stepper This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@Floriferous
Copy link
Contributor

Floriferous commented Jun 5, 2020

I've been struggling to use the Stepper component, as it seems to rely a lot on refs and React.cloneElement. This means that you can't simply compose the component together, as you need to know where and what-for refs, passed props are needed.

Here's a simple reproduction: https://stackblitz.com/edit/w2vekn

I've had to look around multiple files in the Stepper* tree to understand that you can fix the first example by passing any props along to the StepLabel.

I think it'd be simpler if the component used context instead of cloneElement.

The same visual glitches happen if you were to try to pull the Step component inside your own custom one and give that custom component as children to Stepper, if you do that, then you lose the connector.

@Floriferous Floriferous added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 5, 2020
@oliviertassinari oliviertassinari added component: stepper This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 5, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 5, 2020

@Floriferous Agree, it's much easier to use the context now than it was when first designing the component (2017). It's related to #12921. @eps1lon made some progress on it lately #18085. Would you be interested in trying that out?

@oliviertassinari oliviertassinari added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted new feature New feature or request labels Jun 5, 2020
@baterson
Copy link
Contributor

@oliviertassinari I would give it a try :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 10, 2020

@baterson Great, note that it's a larger effort than previously. I think that the first step will be to update all the stepper tests to use testing-library. Then, we can actually look at changing the implementation.

@baterson
Copy link
Contributor

@oliviertassinari Ok then. I'll start with tests first.

@oliviertassinari
Copy link
Member

@baterson Thanks, the reason is that shallow and .find(ComponentName) will be bottlenecks in the update of the implementation. Ideally, we would like to be able to update the implementation without changing a single test.

@baterson
Copy link
Contributor

@oliviertassinari I'm a bit confused on tests like this one:
https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Stepper/Stepper.test.js#L66
Stepper renders div elements instead of Step which causes prop related warnings because div doesn't care about Step props.
Is it ok if I change those divs to Step components?

@oliviertassinari
Copy link
Member

Agree, I think that using the Step would make more sense here.

@mymattcarroll
Copy link

mymattcarroll commented Jun 18, 2020

Is this expected to be a breaking change, and not be available until v5?

We have a use case to make the steps reorder-able, which is proving to be very difficult while the <Stepper /> component must have <Step /> components as children.

@oliviertassinari
Copy link
Member

@mymattcarroll all ongoing development is on v5, yes. We still plan to make releases every week along the way to a stable state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: stepper This is the name of the generic UI component, not the React module! new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants