[Stepper] Refactor Stepper #3903
Conversation
nathanmarks
added
Refactoring
PR: needs revision
nathanmarks
force-pushed
the
stepper-updates
branch
from
2419aed
to
c7efc8f
Compare
nathanmarks
removed
the
PR: needs revision
|
Looks good so far @nathanmarks! |
nathanmarks
force-pushed
the
stepper-updates
branch
from
137c2e2
to
da91a0d
Compare
nathanmarks
changed the title
|
@callemall/material-ui I have 1 or 2 small things to tie up as you can see in the OP, but this is ready for review 👍 |
nathanmarks
force-pushed
the
stepper-updates
branch
from
381ad07
to
e490505
Compare
| @@ -40,9 +41,16 @@ class CodeExample extends React.Component { | |||
| }, | |||
| }; | |||
|
|
|||
| const docs = parse(code); | |||
There was a problem hiding this comment.
Yeah, that feel much better like this.
I think that we should follow this pattern everywhere (having the description above the component).
cc @mbrookes
There was a problem hiding this comment.
Yup, @nathanmarks and I talked about this before he pushed. I like it, and it solves some other problems (string concat vs. backtick escaping) with the current approach. We can fix up the others when this merged. 👍
|
@oliviertassinari I've tended to the items you highlighted 👍 |
| const icon = index + 1; | ||
|
|
||
| return React.cloneElement(child, Object.assign( | ||
| {active, completed, disabled, icon, last}, |
There was a problem hiding this comment.
Personally, I'm a fan of this short-hand syntax for creating objects 😄. Though I think we may have discussed in another issue we may consider setting up an eslint rule to disallow this.
@oliviertassinari Is this fine?
|
@nathanmarks As I'm sure you've noticed, I had mostly nitpick comments. I think this is good for beta release minus that one extra comma in the test. 👍 |
|
@newoga fixed all your points 👍 |
nathanmarks
force-pushed
the
stepper-updates
branch
from
5d56544
to
7111670
Compare
Refactor Stepper to fix issues in the existing implementation that are detailed in mui#3725. Documentation is also reworked for the component and a new transition group added to the internal folder.
nathanmarks
force-pushed
the
stepper-updates
branch
from
7111670
to
8f45c7e
Compare
|
Squashed it 👍 |
|
Alright, everything looks good to me and I just tested this locally. We can review/wrap up the rest of it later. This is what fine looking set of components @nathanmarks! Great work! @namKolo Thanks again for starting this effort 😄 |
| export Step from './Step'; | ||
| export StepButton from './StepButton'; | ||
| export StepContent from './StepContent'; | ||
| export StepLabel from './StepLabel'; | ||
| export Stepper from './Stepper'; |
There was a problem hiding this comment.
Should this be export default from './Stepper' and the imports in he examples be changed to use the default import? I think that's the pattern we're using elsewhere.
There was a problem hiding this comment.
@newoga We are now using https://github.com/leebyron/ecmascript-more-export-from pattern elsewhere. E.g. you can have a look at the index.js.
|
@nathanmarks That PR is awesome. I wish all our components were that quality 👍. |
|
@oliviertassinari thanks! More to come hopefully 😄 |
zannager
added
the
component: stepper
As per #3887 I have been hard at work refactoring
Stepper.I have a few things to finish wrapping up: