Feat/separator #38

jamesducky · 2022-05-03T11:59:29Z

Closes DEVELOP: Separator

Summary of changes included in this PR

Created separator component
Add E2E and unit tests.
Storybook: created a wrapper around the component to allow it to inherit width and height from its parent.
Unable to include color prop due to token naming inconsistencies.
<Hr> element wrapped in a container div with flexbox to allow vertical direction.

Reviewer checklist:

Tests for
- accessibility
- rendered HTML (spec)
Storybook stories for all variants
JSDoc documentation for component
Update Example app (React, ...)

github-actions · 2022-05-03T12:02:57Z

Visit the preview URL for this PR (updated for commit bb62b07):

https://test-auth-d13f2--pr38-feat-separator-4f1bzdny.web.app

_{(expires Mon, 23 May 2022 12:59:33 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

nathan-bird

Tests are failing, please fix.

packages/component-library/src/components/plmg-separator/test/plmg-separator.spec.tsx

packages/component-library/src/components/plmg-separator/plmg-separator.stories.js

nathan-bird

Looks good to me.
Since we pair programmed this I'd like one of the design system devs to also review it.

TanguyDucky · 2022-05-04T11:38:27Z

Please merge main into this branch and update any color token name with color- prefix.
Example: tokens.$plmg-text-neutral becomes tokens.$plmg-color-text-neutral

See #39 for more information.

nathan-bird

With the update to background colour tokens this PR should be updated to include a valid colour prop.

TanguyDucky

Thank you for the pull request! :)
And congrats on your first pull request! 🎉 👯‍♂️

The component looks good but the border-color makes it weird when zooming.
Also I requested some additional stories.

common/config/rush/pnpm-lock.yaml

packages/component-library/src/components/plmg-separator/plmg-separator.scss

packages/component-library/src/components/plmg-separator/plmg-separator.stories.js

packages/component-library/src/components/plmg-separator/test/plmg-separator.spec.tsx

TanguyDucky · 2022-05-04T12:47:22Z

With the update to background colour tokens this PR should be updated to include a valid colour prop.

I've left a comment for designers on Figma. I'd like to ease the constraints on the color prop. Similar to Icon, it should accept any kind of color. Then on the guidelines and the code documentation we can indicate which colors should be used.

jamesducky · 2022-05-05T05:41:47Z

With the update to background colour tokens this PR should be updated to include a valid colour prop.

I've left a comment for designers on Figma. I'd like to ease the constraints on the color prop. Similar to Icon, it should accept any kind of color. Then on the guidelines and the code documentation we can indicate which colors should be used.

Great. Designers agree. I'll add a prop accepting any color.

JakeH91

I have nothing to add. All looks good to me 👍

packages/component-library/src/components/plmg-separator/plmg-separator.tsx

packages/component-library/src/components/plmg-separator/test/plmg-separator.spec.tsx

nathan-bird · 2022-05-09T07:05:02Z

examples/react-app/src/index.tsx

@@ -80,6 +81,10 @@ ReactDOM.render(
          <h1>PlmgCard slot-2</h1>
        </div>
      </PlmgCard>
+      <div style={{ width: '200px', height: '200px' }}>
+        <p>PlmgSeparator - vertical thin</p>
+        <PlmgSeparator thickness="thin" direction="vertical"></PlmgSeparator>


Self closing tag please

examples/react-app/src/index.tsx

…nt per slot Instead of giving multiple elements to the same slot, provide a single one, wrapping the others.

See WICG/webcomponents#624

nathan-bird

Please review how you are applying the styles.

packages/component-library/src/components/plmg-separator/plmg-separator.stories.js

packages/component-library/src/components/plmg-separator/plmg-separator.tsx

packages/component-library/src/components/plmg-separator/plmg-separator.scss

…les. rewrote tests

nathan-bird · 2022-05-12T11:22:37Z

Re-requested a review from Tanguy since he left a review requesting changes previously.

TanguyDucky

All good, just one tiny typo and a comment to update 😊

packages/component-library/src/components/plmg-separator/plmg-separator.tsx

…separator.tsx Co-authored-by: Tanguy <55230735+TanguyDucky@users.noreply.github.com>

TanguyDucky · 2022-05-16T13:03:01Z

Many thanks @jamesducky 🎉

jamesducky · 2022-05-16T13:04:59Z

Thank you @TanguyDucky. 😅

nathan-bird and others added 11 commits May 2, 2022 11:04

init separator

bd5ae58

docs go away

1bb3270

remove color prop

ae374a0

new docs without color prop

ea02dc8

add tests

ef962e6

test docs

47ea859

refactor width prop to thickness

47a6e20

thickness prop docs

7d1930b

flex box to change direction

be2e00c

doc

e79dfcd

added div wrapper to storybook

43bee46

nathan-bird requested changes May 3, 2022

View reviewed changes

James Marriott added 3 commits May 3, 2022 15:33

fixed component tests

0476017

removed unused const in storybook

3990adc

restored deleted CONSTs removed in error

c323302

jamesducky requested a review from nathan-bird May 4, 2022 03:56

nathan-bird approved these changes May 4, 2022

View reviewed changes

nathan-bird requested review from TanguyDucky and JakeH91 May 4, 2022 07:53

nathan-bird requested changes May 4, 2022

View reviewed changes

TanguyDucky requested changes May 4, 2022

View reviewed changes

James Marriott added 4 commits May 5, 2022 11:19

removed wrapper div. add color prop to be any or inherit

12ccdbb

tests fixed

1eb9605

stories updated

1881f27

reverted modified file

9409b07

JakeH91 previously approved these changes May 5, 2022

View reviewed changes

James Marriott added 2 commits May 6, 2022 11:38

put example in react app

b2b557e

fixed test typo

f5e4ef4

nathan-bird reviewed May 9, 2022

View reviewed changes

packages/component-library/src/components/plmg-separator/plmg-separator.tsx Outdated Show resolved Hide resolved

nathan-bird reviewed May 9, 2022

View reviewed changes

packages/component-library/src/components/plmg-separator/test/plmg-separator.spec.tsx Outdated Show resolved Hide resolved

nathan-bird reviewed May 9, 2022

View reviewed changes

packages/component-library/src/components/plmg-separator/test/plmg-separator.spec.tsx Outdated Show resolved Hide resolved

nathan-bird reviewed May 9, 2022

View reviewed changes

examples/react-app/src/index.tsx Show resolved Hide resolved

refactored component

0b60a73

jamesducky marked this pull request as draft May 11, 2022 03:50

TanguyDucky and others added 6 commits May 11, 2022 11:41

fix(example-react): Fix example react app by providing a single eleme…

70f70d7

…nt per slot Instead of giving multiple elements to the same slot, provide a single one, wrapping the others.

fix(self-close): Expand self-closing tag

c9c5a52

See WICG/webcomponents#624

Merge branch 'main' into feat/separator

ec4040c

corrected tests. added all into stories.

bcb11cb

rush build files

08e78db

updated react app. removed self-closing tag

c6505cf

jamesducky marked this pull request as ready for review May 12, 2022 05:27

nathan-bird requested changes May 12, 2022

View reviewed changes

removed background color on container div vertical and associated sty…

6901aa9

…les. rewrote tests

nathan-bird previously approved these changes May 12, 2022

View reviewed changes

nathan-bird requested a review from TanguyDucky May 12, 2022 11:21

TanguyDucky reviewed May 16, 2022

View reviewed changes

packages/component-library/src/components/plmg-separator/plmg-separator.tsx Outdated Show resolved Hide resolved

packages/component-library/src/components/plmg-separator/plmg-separator.tsx Outdated Show resolved Hide resolved

Update packages/component-library/src/components/plmg-separator/plmg-…

c5209ad

…separator.tsx Co-authored-by: Tanguy <55230735+TanguyDucky@users.noreply.github.com>

jamesducky dismissed nathan-bird’s stale review via c5209ad May 16, 2022 12:50

fixed typos

bb62b07

TanguyDucky approved these changes May 16, 2022

View reviewed changes

TanguyDucky merged commit a48d82a into main May 16, 2022

TanguyDucky deleted the feat/separator branch May 16, 2022 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/separator #38

Feat/separator #38

jamesducky commented May 3, 2022 •

edited by nathan-bird

Loading

github-actions bot commented May 3, 2022 •

edited

Loading

nathan-bird left a comment

nathan-bird left a comment

TanguyDucky commented May 4, 2022

nathan-bird left a comment

TanguyDucky left a comment

TanguyDucky commented May 4, 2022 •

edited

Loading

jamesducky commented May 5, 2022

JakeH91 left a comment

nathan-bird May 9, 2022

nathan-bird left a comment

nathan-bird commented May 12, 2022

TanguyDucky left a comment

TanguyDucky commented May 16, 2022

jamesducky commented May 16, 2022 •

edited

Loading

Feat/separator #38

Feat/separator #38

Conversation

jamesducky commented May 3, 2022 • edited by nathan-bird Loading

Summary of changes included in this PR

Reviewer checklist:

github-actions bot commented May 3, 2022 • edited Loading

nathan-bird left a comment

Choose a reason for hiding this comment

nathan-bird left a comment

Choose a reason for hiding this comment

TanguyDucky commented May 4, 2022

nathan-bird left a comment

Choose a reason for hiding this comment

TanguyDucky left a comment

Choose a reason for hiding this comment

TanguyDucky commented May 4, 2022 • edited Loading

jamesducky commented May 5, 2022

JakeH91 left a comment

Choose a reason for hiding this comment

nathan-bird May 9, 2022

Choose a reason for hiding this comment

nathan-bird left a comment

Choose a reason for hiding this comment

nathan-bird commented May 12, 2022

TanguyDucky left a comment

Choose a reason for hiding this comment

TanguyDucky commented May 16, 2022

jamesducky commented May 16, 2022 • edited Loading

jamesducky commented May 3, 2022 •

edited by nathan-bird

Loading

github-actions bot commented May 3, 2022 •

edited

Loading

TanguyDucky commented May 4, 2022 •

edited

Loading

jamesducky commented May 16, 2022 •

edited

Loading