[Stepper] Add new component

[namKolo]

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]

@namKolo Wow, that looks awesome :+1: :+1: Thanks a lot for your contribution :+1: :tada:

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

1. I think Stepper and Step would be better fit, ( personal opinion :grin: )
2. Docs :smile:
3. We are attempting to make this library as composable as possible, so try breaking down these components as much as possible.
4. 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]

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

[mbrookes]

@namKolo - fantastic work! Thanks for sharing with the community. :+1:

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]

@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. :+1:

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. :smile:

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]

@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.

[namKolo]

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

[namKolo]

@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]

@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]

@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]

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

[newoga]

@namKolo Awesome!

[newoga]

@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 ( :tada: :balloon: :heart_eyes: ) 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]

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

[alitaheri]

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

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 :grin:

[newoga]

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 :grin:

Fair enough, ignore what I said @namKolo!

[newoga]

@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. :grin:

[alitaheri]

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 :grin: 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 :grin:

[namKolo]

HorizontalStep =D

[mbrookes]

Very nice work @namKolo ! :+1: :heart:

Could you take a look at the alignment of the content on the Vertical stepper:

The content aligns with the step label.

[namKolo]

@mbrookes @newoga my family are facing to some problems, so i need to solve them. Maybe i will be off about one week. I can't be online to receive your feedback. But i will come back soon (after 1 week) and fix code soon . Thank you so much !

[mbrookes]

@namKola Family is everything. This PR will still be here when you get back. Please take your time!

Thanks for all your hard work.

[newoga]

@mbrookes @newoga my family are facing to some problems, so i need to solve them.

I hope everything is alright. Take your time, family is most important. This PR can wait and we'll continue when you're ready and available. :+1:

Thanks much @namKolo :smile:

[namKolo]

@mbrookes @newoga i updated some code to:

• Pass a function createIcon to create suitable icon based on the state of step
• Remove stepLabel and using stepLabelView to avoid confusing
• Fix alignment based on material ui
• Update content of example in every step Please check it and give me more feedbacks. Thank you very much

[oliviertassinari]

@namKolo I hope you have solved all your problem with your family. Regarding this PR, this is going to be an awesome addition to the lib :100:. However, we are not there yet. You will need to rebase, there is some conflict. I have made some quick feedback on the PR. I can have a closer look later. Thanks :+1:.

[namKolo]

@oliviertassinari Thank you! My family is better now. I will fix your feedbacks and rebase to solve conflict

[newoga]

@oliviertassinari Thank you! My family is better now. I will fix your feedbacks and rebase to solve conflict

Glad to hear, welcome back! :smile:

[mbrookes]

Loving this feature! @namKolo , when you're ready, lets get the last feedback addressed and get this merged. Thanks so much for your hard work pulling this together! :+1:

[namKolo]

@mbrookes i am ready to get the last feedback :) Thanks for your all revisions and feedbacks.

[mbrookes]

If all the rest of the feed back was addressed, I think this is the only thing outstanding:

HorizontalLinearStepper.jsx

+import {
+  Paper,
+  FontIcon,
+  RaisedButton,
+  FlatButton,
+} from 'material-ui';

use individual imports.

Plus a couple of linting issues (Colors are now exported individually).

Also, you could check this off in the ROADMAP :+1:

[namKolo]

@mbrookes i got some errors which belongs to another components when I fetch master and rebase my branch.

[mbrookes]

Can you npm install and try again?

[alitaheri]

Also don't forget to rebase onto master. it was recently fixed.

[namKolo]

@mbrookes wow it works (y) Thank you so much @alitaheri After rebasing on master, i got some bugs, i can't click on any button

[alitaheri]

@namKolo Hmmm maybe you actually do have multiple reacts loaded. look through node_modules and docs/node_modules folder to see if these 2 folders have react in them. remove the one in docs/node_modules if there is any.

Because I'm not seeing that issue on my machine. :grin:

[alitaheri]

@namKolo Any chance you could merge Vertical and Horizontal steps into one Step? and leave the orientation management to Stepper? you can make as many smaller private components as needed to make it simpler. But having two kinds of steps is confusing to users.

[alitaheri]

Please provide a property orientation that can take the values: 'vertical' and 'horizontal'. it's more expressive.

Sorry for nitpicking, this is an awesome component, you did a great job :+1: :+1: Thank you

[namKolo]

@alitaheri To be honest,i really love to hear all your feedbacks. So it's no problem. All your feedbacks i will check and fix it. Thank you so much!

[alitaheri]

Thank you for this awesome component :heart_eyes:

[mbrookes]

DatePicker and TimePicker have a mode property (albeit 'portrait' || 'landscape' rather than 'vertical' and 'horizontal').

Should we use the same prop name here? (Admittedly orientation would have been a better name for those components too, and 'vertical' and `'horizontal' more reusable, but I'm hesitant to suggest another deprecation! Not totally averse, just cautious...)

[alitaheri]

@mbrookes Actually I think we should keep orientation for stepper and mode for pickers as it is. I mean, they express different properties. DatePicker and TimePicker are like pictures and desktop calendars, it makes sense for them to be portrait or landscape. but stepper is like a line, a line can be vertical, horizontal, or else, a line can't be portrait or landscape, it doesn't make sense for it. likewise, a picture or calendar cannot be vertical or horizontal, because that's not intuitive (or is it? O.o)

My point is. I don't think we should make these two have the same prop structure as they represent different behaviors. A DatePicker CAN switch between months or years horizontally or vertically, but it can't be horizontal or vertical itself. A stepper can be smaller on it's main axis because it has more content than it has steps so it can take both forms ( portrait and landscape) depending on it's number of steps or how much content it has.

I think the two enums reflect different things and they shouldn't be mixed. But that's just my opinion. @oliviertassinari @newoga What do you guys think about this?

[mbrookes]

I agree that 'portrait' / 'landscape' are slightly more expressive for DatePicker and TimePicker, but mode is unintuitive. orientation is better both for the pickers, and for Stepper.

However 'vertical' and 'horizontal' in conjunction with orientation do still make sense - 'vertical orientation' == 'portrait', 'horizontal orientation' == 'landscape'.

To summarise the options:

1. Different APIs:
   • Pickers: mode = 'portrait' || 'landscape'
   • Stepper: orientation = 'vertical' || 'horizontal'
2. Same (inexpressive) prop, no deprecation:
   • Pickers: mode = 'portrait' || 'landscape'
   • Stepper: mode = 'vertical' || 'horizontal'
3. Same (expressive) prop, deprecation:
   • Pickers: orientation = 'portrait' || 'landscape'
   • Stepper: orientation = 'vertical' || 'horizontal'
4. Same APIs, deprecation:
   • Pickers: orientation = 'vertical' || 'horizontal'
   • Stepper: orientation = 'vertical' || 'horizontal'

If we want to keep the prop the same (regardless of the parameters), the tradeoff is between less expressive (mode) with no deprecation, and expressive (orientation) with deprecation.

[alitaheri]

:scream: Ok, mode feels wrong when you put it that way :sweat_smile: I understand :+1:

I wish portrait and landscape had a better categorizing name. any chance we can just rename mode? to something like : mood or feel or, i really don't know. This is where my English is exhausted :sweat_smile: :sweat_smile: :sweat_smile: But I believe its best to keep:

Picker: xxx: portrait || landscape (where xxx !== orientation that would be counter-intuitive) Stepper: orientation: vertical || horizontal

[mbrookes]

xxx: portrait || landscape (where xxx !== orientation that would be counter-intuitive)

Why is that counter-intuative? https://en.wikipedia.org/wiki/Page_orientation

[alitaheri]

No no I meant having two sets of value for the same prop name:

Pickers: orientation = 'portrait' || 'landscape' Stepper: orientation = 'vertical' || 'horizontal'

I think this can be confusing, no?

[mbrookes]

That only leaves orientation = 'vertical' || 'horizontal' for both then. It's still a compromise, but it strikes a certain balance.

[alitaheri]

Yeah, I think the convention is worth the compromise. :+1:

[namKolo]

Hmmm, it means that i will provide orientation for user to config which type of stepper they want to use, right?. And no more separate Step into Horizontal and Vertical, only just Step ?

[mbrookes]

@namKolo Sorry for so many comments. This component is soooo good - it might as well be perfect! :laughing:

[namKolo]

@mbrookes sorry for my bad english! Anyway thank for your spending on my code. I am fixing it

[mbrookes]

@nathanmarks - would you mind taking a look at why Travis failed on tests?