Guidelines component #263

cchaos · 2018-01-04T21:27:30Z

This creates a style guide specific component for writing out rows of guidelines.

Basic instance

Includes a top area for a heading and description and two examples (a "do" and a "dont"). Each example takes a type and a text string for a more verbose caption.

Leave the heading and description props blank if you do not needs these displayed.

<GuideGuideline
  heading="Write in active voice"
  description="Active voice puts the focus on who or what is performing the action and makes the sentence easier to understand."
>

  <GuideGuidelineExample type="do" text="">
    <GuideGuidelineWriting>The Elasticsearch Query DSL builds filters.</GuideGuidelineWriting>
  </GuideGuidelineExample>
  <GuideGuidelineExample type="dont" text="">
    <GuideGuidelineWriting>Filters are built using the Elasticsearch Query DSL.</GuideGuidelineWriting>
  </GuideGuidelineExample>
</GuideGuideline>

For simple writing sample examples, use the GuideGuidelineWriting component

Complex example

If you don't want to encapsulate the whole example in panel-style container, add the prop panel={false}. Then you can use your own panel or any other component.

<GuideGuideline
  heading="Avoid using &quot;Are you sure&quot;"
  description="Your text is more direct without it."
>
  <GuideGuidelineExample type="do" text="" panel={false}>
    <EuiPanel>
      …
    </EuiPanel>
  </GuideGuidelineExample>

  <GuideGuidelineExample type="dont" text="" panel={false}>
    <EuiPanel>
      …
    </EuiPanel>
  </GuideGuidelineExample>
</GuideGuideline>

Omitting columns

If you need to have an empty example column, use <EuiFlexItem /> in place of the GuideGuidelineExample.

<GuideGuideline
  heading=""
  description=""
>
  <GuideGuidelineExample type="do" text="">
    …
  </GuideGuidelineExample>

  <EuiFlexItem />
</GuideGuideline>

Section titles

There is also a GuideGuidelineTitle component that will create the correct title component and add the right amount of spacing.

<GuideGuidelineTitle>Style</GuideGuidelineTitle>

snide · 2018-01-05T00:41:46Z

Gonna set up a quick review meeting for this one. I messed around with it for an hour and played around with throwing it into another page. Likely some easy wins we can do to make it a little more flexible. Easier to run through over a zoom.

cchaos · 2018-01-05T20:22:32Z

PR has been updated based on the conversation with @elastic/kibana-design

cjcenizal

Nice improvement! I had a few suggestions about code structure and style.

In terms of the presentation of the do's and don'ts how do you feel about either of these options?

I feel like first option is pretty strong. The benefit of doing either one of these is that our code will become much simpler because we won't need to add custom styles for adding borders to the panels.

cjcenizal · 2018-01-05T22:44:01Z

src-docs/src/components/guide_guideline/_guide_guideline.scss

@@ -0,0 +1,15 @@
+.Guideline {


Can we lowercase the first letter of all of the classes? e.g. guideline

I keep getting tripped up by the repetition of "guide" and "guideline". Can we use the word "rule" instead of "guideline" and add back the general "guide" prefix? So we'd end up with components such as:

GuideRule (with class guideRule)

GuideRuleDescription (with class guideRuleDescription)

GuideRuleTitle (with class guideRuleTitle)

And so on...

cjcenizal · 2018-01-05T22:46:52Z

src-docs/src/components/guide_guideline/_guide_guideline_example.scss

+    .GuidelineExample__panel {
+      border-bottom-color: $euiColorSuccess;
+
+      + small {


Can we use a class instead of an element selector? This will be a little less brittle so the element can change without breaking anything. Also, I'm not sure small is the appropriate element here, semantically speaking. I think it should just be a regular <p>. I think it's typically used for small print like attribution, copyrights, and disclaimers.

What about using <figure> to wrap the whole thing and <figcaption> for the small text?

cjcenizal · 2018-01-05T22:47:32Z

src-docs/src/components/guide_guideline/_guide_guideline_writing.scss

@@ -0,0 +1,3 @@
+.euiGuideGuidelineWriting {
+
+}


Can we delete this since it's a no-op?

cjcenizal · 2018-01-05T22:49:24Z

src-docs/src/components/guide_guideline/guide_guideline.js

+
+  if (description) {
+    descriptionNode = (
+      <GuideGuidelineDescription


Do we need a GuideGuidelineDescription component? I think it's OK to put <EuiText className="guidelineDescription"> and the other stuff in here instead.

I left it separate because I think there may be cases where we would want to add the description without the examples.

cjcenizal · 2018-01-05T22:58:33Z

src-docs/src/components/guide_guideline/guide_guideline_writing.js

+  className,
+  ...rest,
+}) => {
+  const classes = classNames('GuidelineWriting', className);


This class is a no-op, so can we remove this component and just use <EuiText><p>{children}</p></EuiText> in the examples?

I see your point. My thinking was to just make it easy on those who just need a quick example of copywriting. Maybe it can be a helper function inside the writing guidelines instead of it's own component?

cjcenizal

This is looking great! I just had a few last minute suggestions.

cjcenizal · 2018-01-10T19:01:10Z

src-docs/src/components/guide_rule/guide_guideline_example.js

@@ -0,0 +1,67 @@
+import React from 'react';


I think this file needs to be renamed to guide_rule_example.js.

Or can we just delete it? looks like guide_rule_example.js already exists.

Yeah, accidentally left that in

cjcenizal · 2018-01-10T19:02:37Z

src-docs/src/components/guide_rule/guide_guideline_example.js

+      <EuiPanel className={panelClasses}>
+        {children}
+      </EuiPanel>
+      <small>{text || typeToSubtitleTextMap[type]}</small>


👍 I'm OK with this or your figcaption suggestion as long as we're not using element selectors to style them! 😄

cjcenizal · 2018-01-10T19:04:12Z

src-docs/src/components/guide_rule/_guide_rule.scss

+    margin-top: $euiSizeXXL * -2;
+  }
+
+  .euiTitle + &.guideRule--hasHeading {


Hmm this confuses me. What's the use case in which this selector applies? Can I see a screenshot?

Oh never mind, I see it now. Would it be possible to use EuiSpacer instead?

But then you would have to know you need a space and have to know what spacer size.

This way, it always adds the tightens up space before the example row only if it directly follows a title.

I did forget to change the general .euiTitle selector to the GuideRule specific ones so the selector is now .guideRule__title + &.guideRule--hasHeading.

Oh good! That works too.

cjcenizal · 2018-01-10T19:12:28Z

src-docs/src/components/guide_rule/guide_rule_example.js

+        {children}
+      </div>
+    );
+  }


We can make this a little more efficient by conditionally assigning the component type instead of the entire component:

const ChildrenComponent = panel ? EuiPanel : 'div'; const childrenNode = ( <ChildrenComponent className="guideRule__example__panel"> {children} </ChildrenComponent> );

Could I take it further and just put the

<ChildrenComponent className="guideRule__example__panel"> {children} </ChildrenComponent>

directly in the return?

cjcenizal · 2018-01-10T19:14:51Z

src-docs/src/components/guide_rule/guide_rule_writing.js

+  EuiText,
+} from '../../../../src/components';
+
+export const GuideRuleWriting = ({


Maybe it can be a helper function inside the writing guidelines instead of it's own component?

Sounds good!

Is there an example of one that has been created yet that I can look at?

I think I would just cut and paste the code into the top of guidelines/writing.js.

cchaos · 2018-01-10T22:05:38Z

Ok, I think everything has been addressed now.

Again, this will probably be an evolving component as we start building out more doc pages.

cchaos · 2018-01-10T22:09:12Z

Oh except that I am still getting the console error:

Warning: Failed prop type: Invalid prop `component` of value `figure` supplied to `EuiFlexItem`, expected one of ["div","span"].

by trying to use figure as a flex item. We can either let the error stand until #278 is resolved, or I can just add it to the approved list.

cjcenizal

LGTM! I had one small note.

cjcenizal · 2018-01-10T23:10:03Z

src-docs/src/views/guidelines/writing.js

@@ -30,6 +29,23 @@ import {

 import makeId from '../../../../src/components/form/form_row/make_id';

+export const GuideRuleWriting = ({


Can we remove the export here since it's only used in this example?

cjcenizal · 2018-01-10T23:48:29Z

src-docs/src/components/guide_rule/guide_rule.js

+
+import {
+  GuideRuleDescription,
+} from '../../components';


This causes a circular dependency because components/index.js also imports this file. So they end up depending on each other. To solve this we just need to change this path to point to the file containing GuideRuleDescription:

import { GuideRuleDescription, } from './guide_rule_description';

Yeah as soon as I hit comment, I saw it. 😩

Provides descriptions and examples of how to write UI text

Also fixed typos and added don't for contractions

Added more detail and explanations

Added some more don'ts and fixed examples

…y to remove the panel styling from the examples

Also only adding the EuiPanel component if panel=true versus trying to clear out the styles.

cchaos added the documentation Issues or PRs that only affect documentation - will not need changelog entries label Jan 4, 2018

cchaos requested review from snide and gchaps January 4, 2018 21:27

cchaos added the work in progress label Jan 5, 2018

cjcenizal suggested changes Jan 5, 2018

View reviewed changes

cjcenizal suggested changes Jan 10, 2018

View reviewed changes

cchaos force-pushed the writing_guidelines branch from 04ed762 to 4f23b15 Compare January 10, 2018 22:09

cjcenizal approved these changes Jan 10, 2018

View reviewed changes

cjcenizal reviewed Jan 10, 2018

View reviewed changes

gchaps and others added 14 commits January 11, 2018 11:18

Writing guidelines

83433bd

Provides descriptions and examples of how to write UI text

Added code for checkbox

cccbe39

Also fixed typos and added don't for contractions

Incorporated review comments

f37421f

Added more detail and explanations

Second round of edits

4324f2e

Added some more don'ts and fixed examples

Initial work on creating Guideline and GuidelineExample components

dc632bd

Converting to use new Guideline comp

f9fbbb6

Added a GuidelineTitle component

63ead25

Fixing empty example and link in for helpText

7fb00e8

Examples on their own line

708cda2

Pulled writing samples into their own component and adding the abilit…

6718557

…y to remove the panel styling from the examples

Renaming GuideGuideline to GuideRule

cde12d4

Using <figure>

20956bb

Also only adding the EuiPanel component if panel=true versus trying to clear out the styles.

Addressing PR comments

c3cef90

Removing export & fixing circular dependency

92220a7

cchaos force-pushed the writing_guidelines branch from ab5f57e to 92220a7 Compare January 11, 2018 16:19

ƒixing more tests

a8fad78

cchaos merged commit 6f03386 into elastic:master Jan 11, 2018

cchaos deleted the writing_guidelines branch January 11, 2018 17:15

		@@ -30,6 +29,23 @@ import {

		import makeId from '../../../../src/components/form/form_row/make_id';

		export const GuideRuleWriting = ({

Guidelines component #263

Guidelines component #263

Conversation

cchaos commented Jan 4, 2018 • edited Loading

Basic instance

Complex example

Omitting columns

Section titles

snide commented Jan 5, 2018

cchaos commented Jan 5, 2018

cjcenizal left a comment • edited Loading

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos Jan 10, 2018 • edited Loading

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 commented Jan 10, 2018

cchaos commented Jan 10, 2018

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

cchaos commented Jan 4, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading

cchaos Jan 10, 2018 •

edited

Loading