[Stepper] Fix support for no connectors #26874
Conversation
oliviertassinari
changed the title
oliviertassinari
added
bug 🐛
oliviertassinari
force-pushed
the
fix/allow-null-stepper-connector
branch
from
e91127e to
c0192d0
Compare
|
I have rebased on HEAD and migrated the test to TypeScript to have types test coverage (we have a functional test case for this one): @eps1lon Quite interesting to find that one of the test cases was doing nothing (thanks to TypeScript) 😆. Here, the class name doesn't exist: |
| expect(steps[0]).not.to.have.class(stepClasses.active); | ||
| expect(steps[1]).not.to.have.class(stepClasses.active); | ||
| expect(steps[2]).not.to.have.class(stepClasses.active); | ||
| expect(steps[0]).to.have.class(stepIconClasses.active); | ||
| expect(steps[1]).to.have.class(stepIconClasses.active); | ||
| expect(steps[2]).not.to.have.class(stepIconClasses.active); |
There was a problem hiding this comment.
Let's add a new test instead of changing an existing one. These are usually indiciative of breaking changes.
There was a problem hiding this comment.
stepClasses.active === undefined so the assertion was a tautology.
There was a problem hiding this comment.
Why would two icons have the active class when only one step is active?
There was a problem hiding this comment.
The test case renders as:
The first icon is active because activeStep default to 0, and the second one is active because of the extra prop.
There was a problem hiding this comment.
We could remove the test too, not real preferences. My intent was to have the test file migrated to TS, and then, noticed this useless test. This is an attempt to make it meaningful.
There was a problem hiding this comment.
but only one step can be active. This is almost certainly either a bug or an inconsistent state that we should guard against. What you're showing is "completed".
There was a problem hiding this comment.
OK, I think that we are going too off topic to the problem the PR is trying to solve. I will remove this dead test case, without adding a new one.
There was a problem hiding this comment.
For context the initial test we added is identical to the new one I'm proposing #13188.
I would also ask to reconsider your perspective on the problem based on the reactions that the developers had on your past answers on this topic (#13147, #12948)
I personally do think that multiple active steps make sense. When the steps are small enough, the information can be displayed all at once.
There was a problem hiding this comment.
I would also ask to reconsider your perspective on the problem based on the reactions that the developers had on your past answers on this topic
Why would I do that?
I personally do think that multiple active steps make sense. When the steps are small enough, the information can be displayed all at once.
Then don't use a stepper. These have a very specific use case based on user research. You're defeating the purpose of a stepper by displaying multiple steps.
There was a problem hiding this comment.
Are you OK with removing this test (#26874 (comment)) to resolve this thread?
Why would I do that?
My personal experience on coming back to older issues where I got downvotes by community developers has been that I usually missed something. I think that it's because the collective knowledge of the crowd is usually (there are exceptions) superior to individuals. The point I wanted to make is that it can be a signal for an opportunity to challenge his belief system/hypotheses/conclusions.
|
@varandasi Thanks for the fix |
Thank you for the great work. Material UI is awesome. |
The purpose of this PR is to allow to set a null stepper connector in TS. It works in JS. Perhaps there's another way to acomplish this, please let me know..