add Steps component by nreese · Pull Request #202 · elastic/eui

nreese · 2017-12-12T02:11:15Z

Needs some CSS work to use variable instead of hard coded colors and sizes but otherwise is complete

cjcenizal

Looking good! I had a few suggestions but they're all pretty minor.

Did you use the Yeoman generator to create these files? We've found it to be really useful for scaffolding up components, creating the test files, and maintaining consistency among them.

cjcenizal · 2017-12-12T18:26:44Z

src/components/steps/step.js

+import React from 'react';
+import PropTypes from 'prop-types';
+
+export function EuiStep({ children, step, title }) {


Can we add className and ...rest to this destructuring assignment?

Can we add a snapshot test for this component too?

cjcenizal · 2017-12-12T18:26:57Z

src/components/steps/step.js

+
+export function EuiStep({ children, step, title }) {
+  return (
+    <div>


And can we assign them as props to this div?

As well as add a standard component class of euiStep?

cjcenizal · 2017-12-12T18:30:42Z

src/components/steps/steps.js

+function renderSteps(steps, offset = 0) {
+  return steps.map((step, index) => (
+    <EuiStep
+      className="kuiVerticalRhythm"


kui? What is this kui you speak of? :)

Can we also change this logic to allow the consumer to specify className and other arbitrary props on the step object?

function renderSteps(steps, firstStepNumber) { return steps.map((step, index) => { const { className, children, title, ...rest } = step; return ( <EuiStep className={className} key={index} step={firstStepNumber + index} title={title} {...rest} > {children} </EuiStep> ); )); }

cjcenizal · 2017-12-12T18:36:47Z

src/components/steps/steps.js

+
+EuiSteps.propTypes = {
+  className: PropTypes.string,
+  offset: PropTypes.number,


I had to read the docs to understand the role of this prop, but I think a clearer name would help. Maybe something like firstStepNumber?

And can we set defaultProps so this defaults to 1, and remove the default on line 6?

cjcenizal · 2017-12-12T18:43:52Z

src/components/steps/step.js

+        <div className="euiStepNumber">
+          {step}
+        </div>
+        <h3 className="euiStepTitle">


I don't think we can hardcode the h3 here. For accessibility purposes, headings should always be sequential, so if there is an <h3> on the page then there must also be at least one <h2> and only one <h1>.

I think our best option would be to allow the consumer to specify which element gets used for the heading, and to default to <p> if none is specified. This could be specified as a headingElement prop on KuiSteps, since I think we'd want to use the same heading element for each KuiStep within a KuiSteps instance.

We'd probably also want to put the step number inside of the heading element too, if possible, so that screen readers will read the number and heading text together.

Also, can we reuse the EuiTitle component here?

<EuiTitle> <headingElement>{title}</headingElement> </EuiTitle>

cjcenizal · 2017-12-12T23:10:39Z

We should also add a note about this new component to the CHANGELOG.md.

…me and rest to EuiStep

cchaos · 2017-12-13T22:00:59Z

src/components/steps/step.js

+        </EuiTitle>
+      </div>
+
+      <div className="euiStep">


@nreese Can you actually put the .euiStep className on the entire wrapping div and rename this one to be euiStepContent? This way I can know which EuiStep is the last one in the EuiSteps container.

cchaos · 2017-12-13T22:46:21Z

src/components/steps/step.js

+          {step}
+        </div>
+        <EuiTitle className="euiStepTitle">
+          <p>{title}</p>


@nreese Is it possible to propagate what the html tag inside the EuiTitle should be? Meaning, can we add a prop to EuiSteps that is something like titleTag which should be an html tag like 'h2' or 'h3' and defaults to 'p'?

This will help with the DOM flow of headers for accessibility since only when we instantiate the component will we know where in the hierarchy it will lie. I think @cjcenizal mentioned something like this during his review.

cchaos · 2017-12-14T16:41:09Z

👍 LGTM to start styling.

snide

Cool, let's merge down, and then @cchaos can style it up.

cjcenizal

LGTM! I had a few nits but not blockers.

cjcenizal · 2017-12-14T18:32:49Z

src-docs/src/views/steps/heading_element_steps.js

+const steps = [
+  {
+    title: 'inspect me',
+    children: (<h3>{'Did you notice the step title is inside a Heading 2 element?'}</h3>)


Minor nit but I don't think the parens are necessary here.

cjcenizal · 2017-12-14T18:33:38Z

src-docs/src/views/steps/heading_element_steps.js

+    <h1>Heading 1</h1>
+    <EuiSteps
+      steps={steps}
+      headingElement={'h2'}


This can just be "h2" (no braces necessary).

cjcenizal · 2017-12-14T18:33:59Z

src-docs/src/views/steps/steps.js

+const firstSetOfSteps = [
+  {
+    title: 'step 1',
+    children: (<p>{'Do this first'}</p>)


Same nit about parens here and elsewhere in the file.

cjcenizal · 2017-12-14T18:35:07Z

src/components/steps/steps.js

+};
+
+EuiSteps.defaultProps = {
+  firstStepNumber: 0,


Can we set this default to 1? Then you won't need to add 1 on line 20.

cjcenizal · 2017-12-14T18:35:25Z

src/components/steps/steps.test.js

+  },
+  {
+    title: 'third title',
+    children: (<p>{'And finally, do this'}</p>)


Same nit about parens.

add Steps component

3f36a32

nreese requested review from cjcenizal and snide December 12, 2017 02:11

nreese mentioned this pull request Dec 12, 2017

add Steps component #201

Closed

cjcenizal suggested changes Dec 12, 2017

View reviewed changes

rename offset to firstStepNumber, add EuiStep jest test, pass classNa…

1d7124e

…me and rest to EuiStep

cchaos reviewed Dec 13, 2017

View reviewed changes

rename euiStep to euiStepContent, put euiStep class on root element

c150655

cchaos reviewed Dec 13, 2017

View reviewed changes

nreese added 2 commits December 13, 2017 19:23

add headingElement prop

38fba87

add heading element example to src-docs

cf81499

snide approved these changes Dec 14, 2017

View reviewed changes

cjcenizal approved these changes Dec 14, 2017

View reviewed changes

more cleanup

be7c3cd

nreese merged commit 202bd18 into elastic:steps_component Dec 14, 2017

Conversation

nreese commented Dec 12, 2017

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal commented Dec 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchaos commented Dec 14, 2017

Uh oh!

snide left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants