Divider: updated design guidelines #1791

AlbertCarreras · 2021-11-30T20:56:27Z

Summary

Divider: updated design guidelines

Links

[Jira](link to Jira ticket(s))
[TDD](link to Paper doc)
[Figma](link to Figma file)

Checklist

Added unit and Flow Tests
Added documentation + accessibility tests
Verified accessibility: keyboard & screen reader interaction
Checked dark mode, responsiveness, and right-to-left support
Checked stakeholder feedback (e.g. Gestalt designers)

netlify · 2021-11-30T21:01:18Z

✔️ Deploy Preview for gestalt ready!

🔨 Explore the source changes: a617c46

🔍 Inspect the deploy log: https://app.netlify.com/sites/gestalt/deploys/61a91d6dd71ad90008bb4e21

😎 Browse the preview: https://deploy-preview-1791--gestalt.netlify.app

dangerismycat · 2021-12-01T00:12:29Z

docs/pages/divider.js

+    <Page title="Divider">
+      <PageHeader
+        name="Divider"
+        description="If you have two things that need to be separated, put a `Divider` between them."


"things" is a bit generic…perhaps "elements"?

No need for the backticks around Divider — we've moved away from that style for component names

dangerismycat · 2021-12-01T00:13:26Z

docs/pages/divider.js

+  return (
+    <Flex>
+      <Box paddingX={4}>
+        <Block title="Discover ideas" text="Use the searh bar to discover ideas, people and trends. Explore suggested topics or search for topics of your own." />


typo: s/'searh'/'search'

dangerismycat · 2021-12-01T00:13:45Z

docs/pages/divider.js

+      </Box>
+      <Divider />
+      <Box paddingX={4}>
+        <Block title="Create Pins" text="Upload an image from your computer or mobile device to create a Pin. You can also create Pins from images yu find online." />


typo: s/'yu'/'you'

I'm still seeing this typo (here and where the example is repeated below)

dangerismycat · 2021-12-01T00:15:20Z

docs/pages/divider.js

+        <MainSection.Subsection>
+          <MainSection.Card
+            description={`
+Dividers can be a subtle way to separate content, however, white space can be used to accomplish the same goal. Use dividers sparingly to avoid creating unnecessary clutter. Dividers should be used mainly to separate groups of related content.


The first sentence is a bit of a run-on. Perhaps a semi-colon or period between "content" and "however"?

dangerismycat · 2021-12-01T00:15:45Z

docs/pages/divider.js

-card(<PropTable props={[]} />);
+  return (
+    <Flex direction="column" gap={10}>
+      <Block title="Discover ideas" text="Use the searh bar to discover ideas, people and trends. Explore suggested topics or search for topics of your own." />


same typos in these blocks in the various examples

docs/pages/divider.js

dangerismycat · 2021-12-01T00:17:07Z

docs/pages/divider.js

+          <MainSection.Card
+            cardSize="md"
+            type="do"
+            description="Use whitespace primarily to separate groups of related content"


These should end in a period (this example and several others)

dangerismycat · 2021-12-01T00:19:32Z

docs/pages/divider.js

+        <MainSection.Subsection description="The Divider is not focusable and is treated as decorative. Screen readers on tab navigation don't announce Dividers but do announce them on left/right quick navigation." />
+      </MainSection>
+      <MainSection name="Localization">
+        <MainSection.Subsection description="If you are aligning the Divider to content, ensure that it switches sides according to the contents start-alignment" />


"…according to the content's start-end alignment."

apostrophe on "content"

"start-end" to clarify which alignment we're referring to

period at the end

dangerismycat · 2021-12-01T00:21:33Z

docs/pages/divider.js

+}
+`}
+      />
+      <MainSection name="Usage guidelines">


Can this use our standard "Do/Don't" format?

This still needs a "Don't", and part of this "Do" is referring to negative guidance. Can this be split into a "Do"/"Don't" pair?

These are now just repeating the first Best Practices pair. Perhaps work with Kate to develop content that captures the original Usage Guidelines without repeating later content?

DISCUSSED, CHANGED

nit: if we're not pairing it with a "Don't", I think we can forgo the "Do". When not in counterpoint to anything, the "Do" is superfluous

dangerismycat · 2021-12-01T18:09:15Z

docs/pages/divider.js

+      </Box>
+      <Divider />
+      <Box paddingX={4}>
+        <Block title="Create Pins" text="Upload an image from your computer or mobile device to create a Pin. You can also create Pins from images yu find online." />


I'm still seeing this typo (here and where the example is repeated below)

dangerismycat · 2021-12-01T18:10:04Z

docs/pages/divider.js

+}
+`}
+      />
+      <MainSection name="Usage guidelines">


This still needs a "Don't", and part of this "Do" is referring to negative guidance. Can this be split into a "Do"/"Don't" pair?

dangerismycat · 2021-12-01T18:14:28Z

docs/pages/divider.js

+          <MainSection.Card
+            cardSize="md"
+            type="don't"
+            description="Inset the Divider in a way that causes it to be free-floating or separated from content."


nit: no article needed, just "Inset Divider in a way…"

dangerismycat · 2021-12-01T18:16:46Z

docs/pages/divider.js

+    <Flex width="100%" height="100%">
+      <Box paddingX={4}>
+        <Flex direction="column" gap={2} width={120}>
+        {["Public profile", "Account settings", "Home feed tuner", "Claim", "Permissions", "Notifications"].map((item, idx) => <Text underline={idx === 0}weight="bold" key={idx} size="md" wrap="none">{item}</Text>)}


The underline here is a bit odd…I know it's supposed to indicate the currently-selected item, but it reads as a link, so it's unexpected that it's not a link. Perhaps just remove the underline? Or implement using a Box similar to how it's implemented in Pinboard.

DONE, changed to Tabs

dangerismycat · 2021-12-01T18:17:36Z

docs/pages/divider.js

+        </MainSection.Subsection>
+      </MainSection>
+      <MainSection name="Accessibility">
+        <MainSection.Subsection description="The Divider is not focusable and is treated as decorative. Screen readers on tab navigation don't announce Dividers but do announce them on left/right quick navigation." />


nit: no article needed, just "Divider is not focusable…"

dangerismycat · 2021-12-01T18:17:46Z

docs/pages/divider.js

+        <MainSection.Subsection description="The Divider is not focusable and is treated as decorative. Screen readers on tab navigation don't announce Dividers but do announce them on left/right quick navigation." />
+      </MainSection>
+      <MainSection name="Localization">
+        <MainSection.Subsection description="If you are aligning the Divider to content, ensure that it switches sides according to the content's start-end alignment." />


same nit about no article

dangerismycat · 2021-12-01T18:19:24Z

docs/pages/divider.js

+        <MainSection.Subsection
+          title="Orientation"
+          columns={2}
+          description="You can use this component for a visual divider between two elements. Placing it within a [Flex](/flex) layout with a direction of `row` will cause the Divider to become vertical."


The copy seems a little off…perhaps something like:
"You can use this component as a vertical divider between two elements. Placing it within a Flex layout with a direction of "row" will shift Divider to a vertical orientation."

dangerismycat · 2021-12-01T22:51:47Z

docs/pages/divider.js

+}
+`}
+      />
+      <MainSection name="Usage guidelines">


These are now just repeating the first Best Practices pair. Perhaps work with Kate to develop content that captures the original Usage Guidelines without repeating later content?

dangerismycat · 2021-12-01T22:54:29Z

docs/pages/divider.js

+        <MainSection.Subsection columns={2}>
+          <MainSection.Card
+            type="do"
+            description="- Use Dividers or whitespace subt to seperate groups of related content."


I'm not sure what "subt" means here

dangerismycat · 2021-12-01T22:54:40Z

docs/pages/divider.js

+          />
+          <MainSection.Card
+            type="don't"
+            description="- Overuse Dividers as this can lead to uncessary clutter on the page."


typo: "unnecessary"

dangerismycat · 2021-12-02T19:05:07Z

docs/pages/divider.js

+    <Page title="Divider">
+      <PageHeader
+        name="Divider"
+        description="A light gray 1px horizontal or vertical line which groups and divides content in lists and layouts."


ah, missed this previously: this should live as a comment in the component file and get used here as the generated version (generatedDocGen?.description), so users see the description on hover in their IDE

dangerismycat · 2021-12-02T19:06:26Z

docs/pages/divider.js

+}
+`}
+      />
+      <MainSection name="Usage guidelines">


nit: if we're not pairing it with a "Don't", I think we can forgo the "Do". When not in counterpoint to anything, the "Do" is superfluous

fix

5628945

AlbertCarreras added the patch release Patch release label Nov 30, 2021

AlbertCarreras force-pushed the divider branch 2 times, most recently from a221fe7 to ecc9874 Compare November 30, 2021 22:36

AlbertCarreras marked this pull request as ready for review November 30, 2021 23:32

AlbertCarreras requested a review from a team as a code owner November 30, 2021 23:32

design guidelines

ba2f7f2

AlbertCarreras force-pushed the divider branch from ecc9874 to ba2f7f2 Compare November 30, 2021 23:34

dangerismycat reviewed Dec 1, 2021

View reviewed changes

AlbertCarreras requested a review from dangerismycat December 1, 2021 16:41

AlbertCarreras force-pushed the divider branch from 6e13192 to 999a801 Compare December 1, 2021 16:46

comments

0eee7cf

AlbertCarreras force-pushed the divider branch from 999a801 to 0eee7cf Compare December 1, 2021 16:48

dangerismycat reviewed Dec 1, 2021

View reviewed changes

fixes

0316575

AlbertCarreras force-pushed the divider branch from a692504 to 0316575 Compare December 1, 2021 22:45

dangerismycat reviewed Dec 1, 2021

View reviewed changes

changes

134eeeb

AlbertCarreras force-pushed the divider branch from 05230a4 to 134eeeb Compare December 2, 2021 18:45

AlbertCarreras requested a review from dangerismycat December 2, 2021 18:45

dangerismycat approved these changes Dec 2, 2021

View reviewed changes

AlbertCarreras force-pushed the divider branch from 620167f to 901ff0d Compare December 2, 2021 19:14

docgen

a617c46

AlbertCarreras force-pushed the divider branch from 901ff0d to a617c46 Compare December 2, 2021 19:24

AlbertCarreras merged commit b00b1ef into pinterest:master Dec 2, 2021

Divider: updated design guidelines #1791

Divider: updated design guidelines #1791

Conversation

AlbertCarreras commented Nov 30, 2021 • edited

Summary

Links

Checklist

netlify bot commented Nov 30, 2021 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertCarreras commented Nov 30, 2021 •

edited

netlify bot commented Nov 30, 2021 •

edited