Steps are styled #208

cchaos · 2017-12-14T20:22:12Z

Other than just adjusting the CSS, I also wrapped a span with "Step #" in a EuiScreenReaderOnly tag for accessibility. Which then allowed me to put the step number in a data attribute on the title and access that number in CSS to output it into a ::before element. This helps to clean up the output HTML a bit.

…me and rest to EuiStep

# Conflicts: # src-docs/src/views/steps/heading_element_steps.js # src-docs/src/views/steps/steps.js

cchaos · 2017-12-14T20:23:47Z

@cjcenizal or @snide I'm having issues with how this is trying to get merged in. I tried merging in the branch first but got conflicts of added files that were already added, not just line changes...

# Conflicts: # src-docs/src/views/steps/heading_element_steps.js # src-docs/src/views/steps/steps.js # src-docs/src/views/steps/steps_example.js # src/components/steps/__snapshots__/step.test.js.snap # src/components/steps/__snapshots__/steps.test.js.snap # src/components/steps/_steps.scss # src/components/steps/step.js

cchaos · 2017-12-14T20:40:41Z

Alright, the PR is all good now. I'll be better next time to continually rebase, versus merge.

cjcenizal

Looking good! Just had a couple comments, mostly minor but one about accessibility.

cjcenizal · 2017-12-14T21:34:22Z

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

-      </p>
+      <div>
+        <p>
+          To aide with accessibility and hierachical headings,


I think aide (noun) should be spelled "aid" (verb).

Yeah... you might find that you catch a good amount spelling errors with me... I'm trying to be beter...

s/beter/better 😹

Oh and "hierachical" should be "hierarchical"

cjcenizal · 2017-12-14T21:34:34Z

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

@@ -33,7 +37,7 @@ export const StepsExample = {
    demo: <Steps />,
  },
  {
-    title: 'Heading Element Steps',
+    title: 'Heading Elements',


Can we use sentence case?

cjcenizal · 2017-12-14T21:42:53Z

src/components/steps/step.js

-          {step}
-        </div>
-        <EuiTitle className="euiStepTitle">
+        <EuiScreenReaderOnly><span>Step {step}</span></EuiScreenReaderOnly>


When I test the UI with VoiceOver on OS X, it reads "Step 1", "1 , inspect me". So I think a solution similar to one you explored earlier would be better, where "step" is the only word added for the screen reader. That, or "step 1" is added for the screen reader, but the we also hide the visible "1" from the screen reader.

Oh really? I always thought content in pseudo elements were not seen by screen readers.

@cjcenizal Do you think it would be ok to just remove the step number from that line?

snide

Looks good. Very minor comments. Nice work.

snide · 2017-12-14T21:18:51Z

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

-    <h1>Heading 1</h1>
+    <EuiText><h1>Heading 1</h1></EuiText>
+
+    <br/><br/>


Use an EuiSpacer here.

snide · 2017-12-14T21:29:24Z

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

-    title: 'inspect me',
-    children: <h3>Did you notice the step title is inside a Heading 2 element?</h3>
+    title: 'Inspect me',
+    children: <EuiText><h3>Did you notice the step title is inside a Heading 2 element?</h3></EuiText>


Minor one. <EuiText> should be used for blobs of text. In this case, you're only using it for a title, so might want to use <EuiTitle>.

👍 You're right

snide · 2017-12-14T21:33:08Z

src/components/steps/step.js

-        </div>
-        <EuiTitle className="euiStepTitle">
+        <EuiScreenReaderOnly><span>Step {step}</span></EuiScreenReaderOnly>
+        <EuiTitle className="euiStep__title" data-step-num={step}>
          {React.createElement(headingElement, null, title)}


Might want to assign headingElement a default prop value in this file as well as the one that's set in EuiSteps.

Is it not being passed down from EuiSteps to EuiStep?

It is. But I like to think of components as separately usable. Because it's being set as required, you may as well provide it a default. Just a good practice, even if most use cases it would be overwritten by whatever it inherits.

Is it just adding the following to step.js?

EuiStep.defaultProps = { headingElement: 'p' };

snide · 2017-12-14T21:38:27Z

src/components/steps/_steps.scss

-  padding-left: 24px;
-}
+  .euiStep__title::before {
+    content: attr(data-step-num); // Get the number from the data attribute


snide · 2017-12-14T22:09:45Z

src/components/steps/_steps.scss

@@ -1,27 +1,46 @@
-.euiSteps {
+$euiStepNumberSize: $euiSizeXL !default;
+$euiStepNumberMargin: $euiSize !default;


When we've done these component level vars, we've generally put them in the _index.scss file so that they could be used through the various files in the component (can see this in the forms/_index.scss file). Dunno if that makes sense. I'm a little torn on using inner component tokenization when they just inherit global values. They do make things easier to tweak across a component, it's just a slight bit harder to grok when you read the doc.

No changes needed, just kind of voicing random thoughts and seeing if you had opinions.

Here is my thought process:

I create a component level var if the same value (global or custom) will need to be reused in multiple places especially where calculations are made based on that value. For instance:

// Align the content's contents with the title padding-left: ($euiStepNumberSize/2) - 1px + $euiStepNumberMargin;

That way if someone want to change the sizing of the number, they do it in one place and all the alignments and calcs will get updated. Just reduces human error.

Then the only reason they're not scoped to a particular selector is because I needed to reference them in multiple selectors and didn't feel it was necessary to add to the inheritance just for scoped variables.

Works for me. It's more me not liking to have to type everything out 🐻 , but all your points make sense.

cchaos · 2017-12-15T17:21:01Z

Ok I think all comments have been addressed.

cjcenizal

LGTM! But could you address my comments about spelling and sentence case?

cchaos · 2017-12-15T18:14:14Z

@cjcenizal Ugh, I had forgotten to save that file. Ok It's all pushed now.

cjcenizal

🍯 LGTM!

nreese and others added 9 commits December 11, 2017 14:01

add Steps component

3f36a32

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

1d7124e

…me and rest to EuiStep

rename euiStep to euiStepContent, put euiStep class on root element

c150655

add headingElement prop

38fba87

add heading element example to src-docs

cf81499

Styled steps

e81ed86

Updating the docs

0e7778e

more cleanup

be7c3cd

Merge remote-tracking branch 'nreese/steps' into steps-style

8af96ce

# Conflicts: # src-docs/src/views/steps/heading_element_steps.js # src-docs/src/views/steps/steps.js

cchaos added the style only label Dec 14, 2017

cchaos requested review from snide and cjcenizal December 14, 2017 20:22

cjcenizal suggested changes Dec 14, 2017

View reviewed changes

snide reviewed Dec 14, 2017

View reviewed changes

cchaos added 4 commits December 14, 2017 17:44

Spelling and component usage fixes

d0f8bb7

Removed step num from screen reader only elem

35e25f0

Adding default headingElement to step.js

3cea2bc

Updated changelog

906a6bd

cjcenizal reviewed Dec 15, 2017

View reviewed changes

Spelling errors

b446984

cjcenizal approved these changes Dec 15, 2017

View reviewed changes

cchaos merged commit 3e488a0 into elastic:steps_component Dec 15, 2017

cchaos deleted the steps-style branch December 21, 2017 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Steps are styled #208

Steps are styled #208

cchaos commented Dec 14, 2017

cchaos commented Dec 14, 2017

cchaos commented Dec 14, 2017

cjcenizal left a comment

cjcenizal Dec 14, 2017

cchaos Dec 14, 2017

cjcenizal Dec 15, 2017

cjcenizal Dec 15, 2017

cjcenizal Dec 14, 2017

cjcenizal Dec 14, 2017

cchaos Dec 14, 2017

cchaos Dec 14, 2017

snide left a comment

snide Dec 14, 2017

snide Dec 14, 2017

cchaos Dec 14, 2017

snide Dec 14, 2017

cchaos Dec 14, 2017

snide Dec 14, 2017

cchaos Dec 14, 2017

snide Dec 14, 2017

snide Dec 14, 2017

snide Dec 14, 2017

cchaos Dec 14, 2017 •

edited

Loading

snide Dec 14, 2017

cchaos commented Dec 15, 2017

cjcenizal left a comment

cchaos commented Dec 15, 2017

cjcenizal left a comment

Steps are styled #208

Steps are styled #208

Conversation

cchaos commented Dec 14, 2017

cchaos commented Dec 14, 2017

cchaos commented Dec 14, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Dec 15, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cchaos commented Dec 15, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

cchaos Dec 14, 2017 •

edited

Loading