-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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] Refactor Stepper #3903
Conversation
2419aed
to
c7efc8f
Compare
Looks good so far @nathanmarks! |
137c2e2
to
da91a0d
Compare
@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 👍 |
381ad07
to
e490505
Compare
@@ -40,9 +41,16 @@ class CodeExample extends React.Component { | |||
}, | |||
}; | |||
|
|||
const docs = parse(code); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 😄 |
As per #3887 I have been hard at work refactoring
Stepper
.I have a few things to finish wrapping up: