Add Step component

[layershifter]

API

Types

Step

A single step

<div class="ui steps">
  <div class="step">
    Shipping
  </div>
</div>

<Step.Group>
  <Step>Shipping</Step>
</Step.Group>
// or
const items = [
  {text: 'Shipping'}
];

<Step.Group steps={items}>

Groups

Steps

A set of steps

<div class="ui steps">
  <div class="step">
    <i class="truck icon"></i>
    <div class="content">
      <div class="title">Shipping</div>
      <div class="description">Choose your shipping options</div>
    </div>
  </div>
  <div class="active step">
    <i class="payment icon"></i>
    <div class="content">
      <div class="title">Billing</div>
      <div class="description">Enter billing information</div>
    </div>
  </div>
  <div class="disabled step">
    <i class="info icon"></i>
    <div class="content">
      <div class="title">Confirm Order</div>
    </div>
  </div>
</div>

<Step.Group>
  <Step icon='truck'>
    <Step.Title>Shipping</Step.Title>
    <Step.Description>Shipping</Step.Description>
  </Step>
  <Step active icon='payment'>
    <Step.Title>Billing</Step.Title>
    <Step.Description>Confirm Order</Step.Description>
  </Step>
  <Step disabled icon='info' title='Confirm Order' />
</Step.Group>
// or
const items = [
  {icon: 'truck', title: 'Shipping', description: 'Shipping'},
  {active: true, icon: 'payment', title: 'Billing', description: 'Confirm Order'},
  {disabled: true, icon: 'info', title: 'Confirm Order'},
];

<Step.Group steps={items}>

Ordered

A step can show a ordered sequence of steps

<div class="ui ordered steps">
  <div class="completed step">
    <div class="content">
      <div class="title">Shipping</div>
      <div class="description">Choose your shipping options</div>
    </div>
  </div>
...
</div>

<Step.Group ordered>
  <Step completed>
    <Step.Title>Shipping</Step.Title>
    <Step.Description>Shipping</Step.Description>
  </Step>
</Step.Group>
// or
const items = [
  {completed: true, title: 'Billing', description: 'Confirm Order'},
];

<Step.Group ordered steps={items}>

Vertical

A step can be displayed stacked vertically

<div class="ui vertical steps">...</div>

<Step.Group vertical>...</Step.Group>
<Step.Group vertical steps={items}>

Content

Description

A step can contain a description

Already described

Icon

A step can contain an icon

Already described

Link

A step can link

// Case 1
<div class="link step"></div>
// Case 2
<a class="active step">...</a>

// Case 1
<Step link>...</Step>

// Case 2
<Step active href='http://google.com'>...</Step>
<Step active onClick={handler}>...</Step>

States

Active

A step can be highlighted as active

Already described

Completed

A step can show that a user has completed it

Already described

Disabled

A step can show that it cannot be selected

<div class="disabled step"></div>

<Step disabled>...</Step>

Variations

Stackable

A step can stack vertically only on smaller screens

Vertical

A step can be displayed stacked vertically

<div class="ui tablet stackable steps">...</div>

<Step.Group stackable='tablet'>...</Step.Group>
<Step.Group stackable='tablet' steps={items}>

Fluid

A fluid step takes up the width of its container

<div class="ui fluid steps">...</div>

<Step.Group fluid>...</Step.Group>

Attached

Steps can be attached to other elements

<div class="ui top attached steps">...</div>

<Step.Group attached='top'>...</Step.Group>

Evenly Divided

Steps can be divided evenly inside their parent

<div class="ui two steps"></div>

Equal Width

<Step.Group widths='equal'>
  <Step />
  <Step />
</Step.Group>
<div class="ui two steps">
  <div class="step"></div>
  <div class="step"></div>
</div>

Explicit Widths

<Step.Group widths={2}> // or widths='two'
  <Step />
  <Step />
</Step.Group>
<div class="ui two steps">
  <div class="step"></div>
  <div class="step"></div>
</div>

Size

Steps can have different sizes

<div class="ui mini steps">...</div>

<Step.Group size='mini'>...</Step.Group>

Fixes #183

[codecov-io]

Current coverage is 93.76% (diff: 100%)

Merging #335 into master will increase coverage by 0.33%

@@             master       #335   diff @@
==========================================
  Files            68         73     +5   
  Lines           898        946    +48   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            839        887    +48   
  Misses           59         59          
  Partials          0          0          

Powered by Codecov. Last update 4615943...3c89a04

[layershifter]

@levithomason may be cleaner solution?

[levithomason]

Hm, can you elaborate a bit on the goal here? One thing I notice is that the .map((item) => item) looks to be redundant since it just returns every element in the array without modification:

[layershifter]

Refactored this fragment, it might be more understantable now 👍

[levithomason]

I see, it looks like we're abstracting away the Step.Content by creating this automatically. I'm on the fence about this one still. I'll have to give this some more thought. Here are my current thoughts.

There are many components that have content. Usually, content also can contain header, description, meta, and extra component part children. It can also sometimes have additional definition classes, like extra content (Card), image content (Modal), bottom aligned content (Item).

Because of the need to sometimes configure content, and given our sub component API pattern, we may likely end up supporting Content parts something like this:

<Card>
  <Card.Content>...</Card.Content>
  <Card.Content extra>...</Card.Content>
</Card>

<Modal>
  <Modal.Content>...</Modal.Content>
  <Modal.Content image>...</Modal.Content>
</Modal>

<Item>
  <Item.Content>...</Item.Content>
  <Item.Content aligned='bottom'>...</Item.Content>
</Item>

This would mean for the Step:

<Step>
  <Step.Content>...</Step.Content>
</Step>

Again, I'm open for ideas and input here but this is where I'm leaning so far.

EDIT

To be clear, content would be auto created for you when using the props API for creating your component. However, if we go with the above sub component API for Content, you would have to manually wrap your children in Content when appropriate. In most cases, anytime there is markup that is adjacent to the content (an icon, image, etc) then the Content wrapper is required around the header, description, etc. But, if there is no icon/image/etc that is adjacent to the header/description/etc, then there is no content wrapper required. See semantic-ui.com/views/item.html example markup for more.

Linking #325

[layershifter]

@levithomason Seems it make code cleaner, I'll refactor <Content> to separate component

[levithomason]

Sounds good

[layershifter]

I think it's ready for review :)

---

I've updated StepLinkExamples.js, as you can see example with onClick is non-working. Take a look, it seems usage of mapChildType in <StepGroup/> might be updated.

[layershifter]

@levithomason seems there is only one problem to solve, waiting for your opinion

[layershifter]

ping @levithomason

[levithomason]

Taking a look!

[levithomason]

Would you mind rebasing this PR to the latest master? It would help with the review.

[layershifter]

Yes, give me five minutes

[layershifter]

Made 😋

[levithomason]

Thanks much, looking at Feed right now. Will get to this next.

[levithomason]

I think your fork needs to update it's master to the upstream master. I'm still able to review now however.

[levithomason]

Fixing Links

There 2 are changes necessary to fix the links example:

1. StepLinkExamples.js

Spread props on the ClickableStep so the icon, title, and description are passed.

-    return <Step active={this.state.active} onClick={this.handleClick} />
+    return <Step {...this.props} active={this.state.active} onClick={this.handleClick} />

2. StepGroup.js

Because mapChildType will only map Step, the ClickableStep components are filtered out. I've made a few changes to the StepGroup to fix this and some other things:

-if (items) {
-  return (
-    <div {...rest} className={classes}>
-      {items.map((item, index) => <Step key={index} {...item} ordered={ordered} />)}
-    </div>
-  )
-}
-
-return (
-  <div {...rest} className={classes}>
-    {mapChildType(children, Step, (step, index) => (
-      <Step {...step.props} key={index} ordered={ordered} />
-    ))}
-  </div>
-)
+const content = items
+  ? items.map((item, index) => <Step key={index} {...item} ordered={ordered} />)
+  : children
+
+return <div {...rest} className={classes}>{content}</div>

Ordered Updates

Since we no longer map Steps in the Group and add the ordered prop, we need to handle this now. The Step looks like it was only using ordered to wrap the Step title/description in content. I think we just go with your earlier proposal and always wrap title/description props in content. If the user uses subcomponent children, they'll need to wrap it themselves:

Step.js

const {
-  active, className, children, completed, description, disabled, icon, href, link, onClick, ordered, title,
+  active, className, children, completed, description, disabled, icon, href, link, onClick, title,
} = props

-const contentJSX = completed || icon || ordered ? [
-  icon && iconPropRenderer(icon, { key: icon }),
-  children && <StepContent key='content' children={children} />,
-  (title || description) && <StepContent key='content' description={description} title={title} />,
-] : [
-  children && children,
-  title && <StepTitle key='title' title={title} />,
-  description && <StepDescription key='description' description={description} />,
-]
 const StepComponent = href || onClick ? 'a' : 'div'

 return (
   <StepComponent
     {...rest}
     className={classes}
     href={href}
     onClick={handleClick}
   >
-     {contentJSX}
+     {!children && iconPropRenderer(icon)}
+     {children || <StepContent description={description} title={title} />}
   </StepComponent>
 )

For consistency, simplicity, and due to other component conflicts, the children will always be mutually exclusive with props shorthand for v1:

  /** A step can contain an icon. */
-  icon: PropTypes.string,
+  icon: customPropTypes.all([
+    customPropTypes.mutuallyExclusive(['children']),
+    PropTypes.node,
+  ]),

We could technically allow <Step icon='truck'><Title>Shipping</Title></Step> by wrapping children in content when there is an icon prop. Though I want to ship with consistency for v1 and there are components that this is much more difficult or doesn't make sense for. Lastly, after shipping a more straightforward v1 prop API, we can always release non-breaking updates to allow mixing prop markup APIs and subcomponent APIs. Though, we cannot do the same in reverse.

Update Examples

With the above changes, we'll need to use either the props API or subcomponent API, but not both simultaneously. We also need to include the content component when there is an icon and using subcomponents. Example:

StepGroupExample.js

-import { Step } from 'stardust'
+ import { Icon, Step } from 'stardust'

-<Step icon='truck'>
+<Step>
+  <Icon name='truck' />
+  <Step.Content>
    <Step.Title>Shipping</Step.Title>
    <Step.Description>Choose your shipping options</Step.Description>
+  </Step.Content>
</Step>

---

With all these changes, everything seems to work in the docs and components. It also seems more simple, which is good. LMK how the updates go!

[layershifter]

Thanks for review.

Fixing Links

1. StepLinkExamples.js

Agree, it works.

2. StepGroup.js

items.map((item, index) => <Step key={index} {...item} ordered={ordered} />)

Antipattern with key is there 🚒 I think about extended validation of items prop with PropTypes.shape and form key from them:

const {description, title} = item
const key = `${title}-${description}`

Ordered Updates

I don't like these updates because they will break this nice layout:

<Step icon='truck'>
  <Title>Shipping</Title>
  <Description>Choose your shipping options</Description>
</Step>

May be we can use React.Content for passing ordered prop to children. Is any reason to reject this solution (except, StepGroup will cease to be stateless)?

[levithomason]

...I think about extended validation of items prop with PropTypes.shape and form key from them

That is much better. We can allow them to pass a key in the items array. How about something like this (accounts for undefined title/description):

items.map((item) => {
  const key = item.key || [item.title, item.description].join('-')
  return <Step key={key} {...item} />
})

I don't like these updates because they will break this nice layout...

TL;DR at the bottom 😄

The markup Step > Title, Description is definitely cleaner than Step > Content > Title, Description. However, consider other components with content. The content wrapper is usually required anytime there is an adjacent icon or image:

<h2 class="ui header">Account Settings</h2>

<h2 class="ui icon header">
  <i class="settings icon"></i>
  <div class="content">
    Account Settings
  </div>
</h2>

<div class="ui card">
  <div class="image">
    <img src="/images/avatar2/large/kristy.png">
  </div>
  <div class="content">...</div>
</div>

<div class="ui comments">
  <div class="comment">
    <a class="avatar">
      <img src="/images/avatar/small/joe.jpg">
    </a>
    <div class="content">... </div>
  </div>
</div>

At first, it seems safe to abstract away the content wrapper based on the presence of an icon or image prop. Though, we'd have to accept some limitations. The user will never be able access the content wrapper nor insert any markup between the root node and the content node. We are assuming then that our API will cover all possible use cases of markup between the root node and content. We are also assuming that our props API can handle all use cases for props on components between this space. We are removing access to this part of the render tree as users are limited to passing nodes through the icon/image props.

I see three places this could break down and cause issues. First, if a user needed to pass something more complicated than a basic icon name or image src through the props then it could get ugly and painful. Second, I think it would be an issue if only some of our components allowed use of icon/image/etc prop with children and automatically inserted the content wrapper, while other components did not allow icon/image/etc with children and required the user to manually insert the content wrapper. I think consistency in the library should take priority over convenience. Last, if the user ever wanted/needed to add props to the content, then it could be an issue as we'd have to also accept content props through a component prop.

I think all of these issues are already present. At TA we had a fair number of use cases where we had to pass complicated components through shorthand props that fully abstracted away access to parts of the render tree. It made for very ugly code and markup issues that would not be present if we had full access and control to the component markup.

We also have components in the library with content parts that cannot be easily configured with only an icon/image prop value. Modal content is adjacent to a header. Imagine the user wants to add a header close icon with a click handler to close the modal. (We had a similar issue at TA with passing markup through <ListItem icon='...' />)

const header = (
  // extra node to wrap text :( sometimes breaks styles
  <span>
    Here is my header
    <Icon style={closeIconStyle} name='close icon' onClick={this.hideModal} />
  </span>
)

<Modal header={header}>...</Modal>

<div class="ui modal">
  <div class="header">
    <span>
      Here is my header
      <Icon name='close icon' />
    </span>
  </div>
  <div class="content">...</div>
</div>

Whereas, if we did not abstract away access to the content node, then the user could have simply added the Icon. Also, the generated markup does not require any extra span node. (The same is true in the case of a ListItem with extra markup in the icon/image part of the tree):

<Modal>
  <Modal.Header>
    This is my header
    <Icon style={closeIconStyle} name='close icon' onClick={this.hideModal} />
  </Modal.Header>
  <Modal.Content>
    ...
  </Modal.Content>
</Modal>

<div class="ui modal">
  <div class="header">
      Here is my header
      <Icon name='close icon' />
  </div>
  <div class="content">...</div>
</div>

There are also components which take props on the content. Like vertically aligned lists, modals with image content, animated buttons, and probably others. These could be hoisted up to the top level, but this again is making the assumption that there will never be conflicting props between the top component and the content.

TL;DR

In conclusion, completely abstracting away Content offers cleaner markup for simple use cases. Anything beyond simple use causes problematic and extra complicated markup. The Content subcomponent API is more verbose, but does not become problematic or complicated for use cases beyond the simple. Mixing the props API with the subcomponent API removes the benefits of the subcomponent API while adding the issues of the props API.

Because of this, I think our first version should offer the props API for simple use cases, offer the subcomponent API for use cases beyond the smiple, and make these two mutually exclusive.

---

P.S.

Personally, I like mixing the props API and sub component APIs to abstract away Content. I'd be very happy to see a solution that does this while also solving the above noted issues. My colleagues and I have not yet figured this out and I haven't seen a proposal that does. I would accept an API proposal that does this. If not, I hope it is figured out for v2.

[layershifter]

@levithomason thanks for a detailed point of view 👍

[levithomason]

No problem, explaining helps me to better understand it myself!

[levithomason]

Once the step/examples are updated, I think we can merge this one

[layershifter]

I've updated all, so let's go 🚗

[levithomason]

Woop! I'll make some doc updates soon. Need to remove ordered from Step propTypes. I also started adding descriptions to the props API variant of the docs per these comments.