[Stepper] Add new component #3132

namKolo · 2016-02-02T08:06:34Z

Closes #2722.

I just created**_ Vertical Stepper_** include animation, linear step, non-linear step, optional step.
To be clear and easy to handle logic how to active a step and when the step is completed, the component delegate the implementation of those function to user(who use this component) to handle logic by themselves.
This is a picture to demonstrate how it work

Please take a look at this component.
Thank you so much :)

alitaheri · 2016-02-02T10:10:31Z

@namKolo Wow, that looks awesome 👍 👍 Thanks a lot for your contribution 👍 🎉

This is certainly a good start but there are some things I'd like to point out before other maintainers review too.

I think Stepper and Step would be better fit, ( personal opinion 😁 )
Docs 😄
We are attempting to make this library as composable as possible, so try breaking down these components as much as possible.
We are converting the naming convention, so please create a folder structure like this:

- src
  - Stepper
    - Stepper.jsx
    - Step.jsx
    - index.js ( re-export Stepper)

You don't really have to take care of these all by your self. I'm sure we can help out. In fact. I think it might not be a bad idea to do a premature merge, before documenting it so we can tackle with it as much as we can, before publishing it. @oliviertassinari @newoga @mbrookes What do you guys think?

namKolo · 2016-02-02T10:21:15Z

Thank you so much for your attention. I will write Docs for this component soon.
I thought about how to be easy composable a lot. Then i figured out I just passed activeStepIndex down Stepper. Based on this property, stepper will know how to render the view again. Stepper doesn't care about the logic in choosing which step will be active next. Moving forward and Cancel step makes the same things.
User will handle that logic by himself. So it can apply for non linear stepper, linear stepper (include optional steps) easily

oliviertassinari · 2016-02-02T13:08:37Z

src/Stepper/Step.jsx

+
+  contextTypes: {
+    muiTheme: PropTypes.object,
+    completedIcon: PropTypes.node,


To be confirmed, but I think that React is going to allow only one property to be passed with the context.
Can't we use a property here?

Yeah I'd also prefer to find a way to use a prop here if possible

mbrookes · 2016-02-02T13:49:03Z

@namKolo - fantastic work! Thanks for sharing with the community. 👍

Whilst a horizontal stepper isn't part of this PR, I'm wondering how it will be accommodated in the future?

Is there an opportunity to separate the logic in the Step component, from the presentation in an internal VerticalStep component, leaving the possibility to add a HorizontalStep or even possibly MobileStep?

newoga · 2016-02-02T15:53:26Z

@namKolo Very cool and excellent job maintaining code consistency and following the same patterns the other components are using. It makes this much easier to follow. 👍

I'll review code and API soon.

You don't really have to take care of these all by your self. I'm sure we can help out. In fact. I think it might not be a bad idea to do a premature merge, before documenting it so we can tackle with it as much as we can, before publishing it.

I'm fine with this. I really want to review this closely. It's such a cool component I want to make sure we get it right the first time. 😄

Actually, on second thought, could we have @namKolo work on some examples in docs? I recommend we can create a route for it without linking to it on the LeftNav. It's just super convenient as I'm switching branches to have an example that is rendered on the doc site that I can get to.

namKolo · 2016-02-02T15:55:42Z

@newoga thanks for your feedbacks, i will follow convention. And I am working on examples for this component in docs. I will push it soon.

newoga · 2016-02-02T15:56:43Z

src/Stepper/Step.jsx

+     the step is last step or not (which passed by automatically from Steps wrapper parent, no need
+     to pass directly)
+    */
+    isLastStep: PropTypes.bool,


@alitaheri Are props like this what we're planning to use the @internal tag for? Or @ignore?

it's @ignore 😁

namKolo · 2016-02-02T16:38:30Z

@newoga i pushed the example as you said.
The new route : http://localhost:3000/#/components/stepper
Please take a look at this :)

namKolo · 2016-02-02T16:40:41Z

@mbrookes i will try a HorizontalStep and push it.
Maybe the logic like you said. I will separate it to HorizontalStep and VerticalStep
Thanks for your attention

newoga · 2016-02-02T16:51:13Z

@newoga i pushed the example as you said.
The new route : http://localhost:3000/#/components/stepper
Please take a look at this :)

Great, thanks!

newoga · 2016-02-04T08:06:41Z

@namKolo It's 3AM my time, I just tried your branch out and I think this is super cool! I'm sorry it's taking us some time to review, I promise this isn't being ignored. I haven't had a chance to thoroughly go over the code an API but it's on my list.

namKolo · 2016-02-04T08:09:08Z

@newoga thank you so much for your reviewing :) By the way, I will push Horizontal Stepper soon. It's nearly finished

newoga · 2016-02-04T08:09:53Z

@namKolo Awesome!

newoga · 2016-02-04T08:19:21Z

src/Stepper/Step.jsx

+    children: PropTypes.node,
+
+    /**
+     *Override the inline-styles of div which contains all the children include control button groups


Sorry! This is a Nitpick, could you add a space in each line before starting the comment? 😁

/** *Good */ /** * Better */

Let 's me add that space =D

newoga · 2016-02-04T09:06:55Z

@oliviertassinari @alitaheri Should we just have @namKolo write the <Step /> component as a functional component, and get the prepareStyles() from context?

I'm noticing now that he already wrote Step to be stateless ( 🎉 🎈 😍 ) so it should be an easy switch. I was originally thinking we could just do it later, but @namKolo has been doing a good job thus far following the style conventions of our components and we have a few components like <Divider /> and <Subheader /> he can look at for reference.

Edit: @namKolo, just to give you some context, we're going to moving towards functional and class based components, and away from mixins. So even though you did an excellent job following the general code style, it's something we're planning on moving away from soon.

namKolo · 2016-02-04T09:11:05Z

@newoga it means i shouldn't use mixin any more ? Get them through context types?

alitaheri · 2016-02-04T09:13:31Z

@newoga Yeah this is surprising well done for a new component and a first contribution. well done @namKolo 👍 👍

Should we just have @namKolo write the component as a functional component, and get the prepareStyles() from context?

I think it would be better if we did it all in one big PR so we can be sure that approach has no caveats. for now it's better to stick to the old ways until we make sure it works as expected 😁

newoga · 2016-02-04T09:15:21Z

I think it would be better if we did it all in one big PR so we can be sure that approach has no caveats. for now it's better to stick to the old ways until we make sure it works as expected 😁

Fair enough, ignore what I said @namKolo!

newoga · 2016-02-04T09:18:51Z

@alitaheri Just for clarification, do you think the prepareStyles from context change should also be done in one big PR? Or just the move to functional components with wrappers? Both?

Only reason why I asked is because I started doing the prepareStyles/remove style-propable removal piecemeal in #2909. If you think they shouldn't be done together then maybe I can close or expand that PR. 😁

alitaheri · 2016-02-04T10:07:50Z

Or just the move to functional components with wrappers? Both?

How do you mean? O.o

Only reason why I asked is because I started doing the prepareStyles/remove style-propable removal piecemeal in #2909.

That's not related to the components. I meant for the components, because it's gonna be one huge find and replace for most cases 😁 I think it's best to keep that PR as is, and submit another one replacing all usages in components all at once, since it's more straight forward. although that's just an opinion. I'm happy with anything that works 😁

mbrookes · 2016-03-01T14:16:31Z

For me, the number is centered already hmm

Could be a browser compatibility issue (Google Chrome), I'll have another look after the next update.

Just a gentle reminder, there are a couple of comments still to be addressed: https://github.com/callemall/material-ui/pull/3132/files

Also, please check this off in the ROADMAP so that it gets updated when this gets merged.

nathanmarks · 2016-03-01T14:17:24Z

@namKolo Feel free to reach out if you need any additional help wrapping this up. I'm here as a resource for you.

namKolo · 2016-03-01T14:28:32Z

@nathanmarks 👍 thank.
I have a small question. After changing the import statement as you said. I ran npm run test and no longer had warnings more.
But when i ran npm run start in docs, it throws me warning.

P/S I figured out why i failed. I forgot to change stepper -> Stepper in some places. Pls ignore this comment

mbrookes · 2016-03-01T15:08:00Z

src/Stepper/Stepper.jsx

+    horizontal: PropTypes.bool,
+
+    /**
+     * Callback function that is fired when the header of step is touched.


"...when the step header is touched."

Sorry, missed this one.

mbrookes · 2016-03-02T22:58:55Z

Okay, I think this is beyond ready to merge. 👍

@namKolo you've been very patient with all our suggestions. Please squash (you can make the commit title match the PR title), then if there's anything else we can pick it up after it's merged.

namKolo · 2016-03-03T13:37:29Z

👍 nice, i will squash soon

namKolo · 2016-03-04T17:21:47Z

I squashed. If there are any suggestions or any feedback, please tell me. Thanks

[Stepper] Add new component

mbrookes · 2016-03-04T17:35:40Z

Yee haw! 👍

Thanks so much @namKolo 🎉 ✨

alitaheri · 2016-03-04T17:42:21Z

This is a great addition to the collection 👍 Thanks a lot for your awesome work @namKolo 😍 😍 🎉 🎉 🍻 👍

newoga · 2016-03-04T17:53:52Z

@namKolo Great job! You're welcome to contribute any time! 😄 🎉

igorpreston · 2016-03-04T18:00:51Z

Hey, when we will be able to use the stepper component in material-ui? Really looking forward to it! Thanks!

newoga · 2016-03-04T18:03:20Z

@igorpreston You can use it in master now. It will be included in our next alpha release, but we don't have a timeline for that yet.

lucasjans · 2016-03-05T01:19:59Z

Nice work @namKolo - a very critical component for our UX flow. Thank you very much for doing this!

e-karma · 2016-03-05T07:02:04Z

@newoga Hello all and i'd like to say thanks for all the hard work! I'm very excited to try it out but unfortunately have had no luck after a few hours of trying to integrate it to a project. Very likely could be due to my inexperience porting from part a commit but experienced a few errors. Thanks again!

mbrookes · 2016-03-05T10:31:52Z

Please ask in SO or gitter.

[Stepper] Add new component

oliviertassinari reviewed Feb 2, 2016
View reviewed changes

mbrookes mentioned this pull request Feb 2, 2016

[Components] supported material-ui components #2863

Closed

newoga reviewed Feb 2, 2016
View reviewed changes

lucasjans mentioned this pull request Feb 3, 2016

Material design Stepper for Material UI #2722

Closed

oliviertassinari added the PR: Needs Review label Feb 3, 2016

newoga reviewed Feb 4, 2016
View reviewed changes

namKolo force-pushed the master branch from 1f820de to 555f83a Compare March 1, 2016 14:49

mbrookes reviewed Mar 1, 2016
View reviewed changes

mbrookes changed the title ~~[New feature] Stepper material ui~~ [Stepper] Add new component Mar 2, 2016

mbrookes self-assigned this Mar 2, 2016

[Stepper] Add new component

aaf275f

namKolo force-pushed the master branch from 15fbc63 to aaf275f Compare March 3, 2016 16:44

mbrookes added a commit that referenced this pull request Mar 4, 2016

Merge pull request #3132 from namKolo/master

57bd22f

[Stepper] Add new component

mbrookes merged commit 57bd22f into mui:master Mar 4, 2016

mbrookes added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 4, 2016

mbrookes added the New Component label Mar 5, 2016

mbradleyis pushed a commit to staticinstance/material-ui that referenced this pull request Mar 7, 2016

Merge pull request mui#3132 from namKolo/master

1614327

[Stepper] Add new component

nathanmarks mentioned this pull request Mar 17, 2016

[Stepper] A few recommendations to clean up the API before the 0.15.0 release #3725

Closed

mbrookes mentioned this pull request May 8, 2016

[Docs] Update the Roadmap #4205

Closed

zannager added the component: stepper This is the name of the generic UI component, not the React module! label Mar 14, 2023

[Stepper] Add new component #3132

[Stepper] Add new component #3132

Conversation

namKolo commented Feb 2, 2016

alitaheri commented Feb 2, 2016

namKolo commented Feb 2, 2016

oliviertassinari Feb 2, 2016

Choose a reason for hiding this comment

newoga Feb 2, 2016

Choose a reason for hiding this comment

mbrookes commented Feb 2, 2016

newoga commented Feb 2, 2016

namKolo commented Feb 2, 2016

newoga Feb 2, 2016

Choose a reason for hiding this comment

alitaheri Feb 2, 2016

Choose a reason for hiding this comment

namKolo commented Feb 2, 2016

namKolo commented Feb 2, 2016

newoga commented Feb 2, 2016

newoga commented Feb 4, 2016

namKolo commented Feb 4, 2016

newoga commented Feb 4, 2016

newoga Feb 4, 2016

Choose a reason for hiding this comment

namKolo Feb 4, 2016

Choose a reason for hiding this comment

newoga commented Feb 4, 2016

namKolo commented Feb 4, 2016

alitaheri commented Feb 4, 2016

newoga commented Feb 4, 2016

newoga commented Feb 4, 2016

alitaheri commented Feb 4, 2016

mbrookes commented Mar 1, 2016

nathanmarks commented Mar 1, 2016

namKolo commented Mar 1, 2016

mbrookes Mar 1, 2016

Choose a reason for hiding this comment

mbrookes commented Mar 2, 2016

namKolo commented Mar 3, 2016

namKolo commented Mar 4, 2016

mbrookes commented Mar 4, 2016

alitaheri commented Mar 4, 2016

newoga commented Mar 4, 2016

igorpreston commented Mar 4, 2016

newoga commented Mar 4, 2016

lucasjans commented Mar 5, 2016

e-karma commented Mar 5, 2016

mbrookes commented Mar 5, 2016