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

[nathanmarks]

TLDR: I think some changes are needed before 0.15.0 is released.

@callemall/material-ui please let me know what you think to the points below. I am willing to take a lot of this on but need your input first.

<Stepper />

The use of <Avatar /> and createIcon prop.

One mistake that was made in the component was the hard dependency on <Avatar />. Users need to be able to use any type of icon, not just an icon placed inside an avatar. I believe that Google's intention for the check in the circle was to simply use the check_circle icon: (With <Avatar /> being used for numbers or letters).

Esp when you compare our checks to the spec examples:

The problem with just placing icon components inside avatars is that it makes it annoying to recreate examples such as this using icons with different shaped backgrounds:

I also question the API design with regards to icon selection. At the present time, icons are set using the createIcon callback function passed to the <Stepper /> component, despite the <Stepper /> not displaying any icons itself. It makes much more sense (IMO) to place an icon prop on the <Step /> itself for a more familiar API design.

Another issue that has stemmed from the internal use of <Avatar /> by default is some confusing prop names and descriptions, for eg:

updateAvatarBackgroundColor
Callback function fired on re-render to set the background color of the icon. If not passed, it will use the default theme.

There's actually a couple of issues with this prop. First off, Avatar is the wrong terminology to be using when referring to icons in the stepper. By definition an avatar is an icon or figure representing a specific person. Secondly, similar to a point I raised about the createIcon callback, this seems a little backwards in design. It would be much more appropriate IMO to set the color of the icon on the <Step /> itself, rather than a callback function in the <Stepper />.

Questionable props

I mentioned a couple of these in the previous section, but there are a few more examples of props using this callback design where it's either confusing or not idiomatic.

For eg:

updateCompletedStatus
Callback function fired on re-render to update the completed status of the step.

I wasn't even sure at first what this prop even did, but then I saw it is a helper callback, similiar to the createIcon callback that basically asks you whether a <Step /> is completed or not when looping through the children:

isCompleted: updateCompletedStatus(index, step)

Again, I'm struggling to see the purpose of this when it's easier and more idiomatic to pass in the isCompleted (could just be completed) prop to the <Step /> itself. I think the <Stepper /> is doing a little too much here and creating confusion.

I'm also a little confused by the handling and propagation of navigation events. We have this property on the <Stepper />:

onStepHeaderTouch
Callback function fired when the step header is touched.

This callback returns the stepIndex and the origin <Step /> node. Which is an odd signature.

It's also misleading, as when the function is passed down to the <Step /> component, it's not put in onTouchTap:

onClick={this.handleStepHeaderTouch}

This isn't an uncontrolled component, so rather than having this confusing propagation chain, why not add onTouchTap or onClick, whatever catches your fancy, to the <Step /> itself? As it stands, you cannot even override the onClick handler for a specific step the way this is setup with the parent cloning the components and overriding props -- you're forced to use it through the <Stepper /> callback. (no ...other either).

On other note on props: Is there any way to reduce the number of style props on the <Step />? Right now, the vertical step has 9 props, 6 of which are style props:

actions
actionsWrapperStyle
children
childrenWrapperStyle
connectorLineStyle
stepContainerStyle
stepHeaderStyle
stepHeaderWrapperStyle
stepLabel

Last but not least, we have a prop on the <Stepper /> that, with no indication, is only actually used on the horizontal stepper:

containerStyle
Override the inline-style of the content container.

Only place it is used is in getStylesForHorizontalStepper().

Styling issues and incorrect/overlooked spec implementation

There are a number of styling bugs littered around such sizing, as seen here:

Overlapping content during animation: (this is way more noticeable when you have more content in your steps)

We're incorrectly styling the non-linear steps, they are supposed to get different treatment from the linear steps. Here is our non linear example, as you can see, the other steps are faded out:

If we look at the MD spec, it tells a different story:

There's also a styling issue with regards to active and completed step states.

In our component, a completed step does not have any extra styling on the label:

However, the MD spec again tells a different story for treatment here. Linear steppers bold the active step, whereas the non-linear steppers bold both active and completed steps.

There's also a key styling option missing:

The ability to create a horizontal stepper with labels in-line rather than underneath the icons, as seen in most of the examples. This is not possible.

In fact, the labels underneath setup is presented in the spec as an "Alternative label placement":

There's a use of display: table-cell that we should remove. The parent of the stepHeaderWrapper has display: flex;, yet we're using currently display: table-cell on the wrapper in the HorizontalStep component.

Looking even closer at styling in HorizontalStep there are a few other small issues that should be resolved. For eg, hard coded colours should be in the theme:

Opinionated Horizontal animation and no way to remove it

The slider-style horizontal animation is not in the spec, and is not used in several examples I've seen in the wild. Many would argue that having slider style animation with content that gets cut off at an overflow: hidden point (which this has) is less than ideal UX. (The only animation the stepper spec for horizontal shows fading and a loader for a custom loading step)

There's no way to override this particular transition prop so we're forced into using it. Personally, using this in my App I'd rather do a fade to avoid cutting off content.

Use of <Paper />

We shouldn't use <Paper /> as a container for steppers inside the component. Steppers can exist outside of paper, (especially needed on mobile). This was only applied to the horizontal steps, but I believe that my reasoning still applies. There are times when you may even put your stepper inside a card or a dialog, where having a <Paper /> container would actually violate MD guidelines for layering depth/paper.

Horizontal/vertical stuff

Something I noticed (compared to the MD spec) in this component that the default configuration is a vertical stepper and that horizontal layout has to be specified via a prop. In the MD spec, "stepper" mostly refers to a horizontal stepper whereas "vertical stepper" is used to refer to a vertical stepper. Our setup here is the reverse of this relationship. Obviously, this point could be a little subjective.

I also believe that defining the <VerticalStep /> and <HorizontalStep /> as different component types isn't necessary, as you'll never be using a combination of the two in the same <Stepper /> and they require the same core functionality.

It makes more sense to just rely on the <Stepper vertical={true} /> to define the type of stepper layout since there is already a coupling between the two components.

The <Stepper /> even has two separate rendering functions for vertical vs horizontal. So I think something has to give here.

@alitaheri actually brought this up in the original PR -- so much was going on it got lost at sea though: #3132 (comment)
#3132 (comment)

[mbrookes]

The problem with just placing icon components inside avatars is that it makes it annoying to recreate examples such as this using icons with different shaped backgrounds

I agree. We can make that prop accept a string or element, and not wrap in avatar for the latter.

It makes much more sense (IMO) to place an icon prop on the itself for a more familiar API design.

I think (if I'm understanding correctly based on our previous conversations) that you are also suggesting that the icon prop should accept an element, and not a callback function. Is that correct?

On other note on props: Is there any way to reduce the number of style props on the ? Right now, the vertical step has 9 props, 6 of which are style props:

Any styles that are static once the component has been rendered should be keys in a single object prop, rather than separate props IHMO, (for this any other component with chronic stylePropItis).

Horizontal/vertical stuff

Already mostly fixed in my fork.

Everything else I basically agree with, so I'm not going to litter this with a list of quotations and _"I agree"_s. 😄

[namKolo]

wow, i love all your feedbacks!!! it really helps me a lot !!! 👍

[nathanmarks]

@namKolo

No problem!! I am currently working on updating the code and shuffling some things around. I think you'll like the end result 👍

[nicolanicola]

Thanks for this. I'm getting errors however this.context.createIcon is not a function and updateCompletedStatus is not a function. Anyway to resolve these? Would love to use this in my project.

[nathanmarks]

hey @nicolanicola ,

This component is not fully released yet, my recommendation would be to wait until 0.15.0 is released as the API is going to change before then.

The changes will solve your issues btw.

[namKolo]

@nathanmarks yeah!!! i really love it :) hope to see it soon.
btw thank you !!!

[nathanmarks]

removing from milestone in favour of tagging the PR