Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE-771] Add Box, Flex, and Stack #1027

Merged
merged 66 commits into from
May 25, 2021

Conversation

alanbsmith
Copy link
Member

@alanbsmith alanbsmith commented Apr 16, 2021

Issue

Resolves #771

Overview

This PR primarily adds Box, Flex, and Stack components to canvas-kit. Below is a list of more detailed changes:

  • Create Box component in labs/common
    • Add Box docs, unit tests for style props, and visual regression tests
  • Create Flex component in labs/layout
    • Add Flex docs, unit tests for style props, and visual regression tests
  • Create Stack, HStack, and VStack components in labs/layout
    • Add Stack docs, unit tests for style props, and visual regression tests
  • Remove space style function from labs/tokens
    • Update 5.0 Migration Guide about the removal
  • Update border-radius zero token to be "0px" from 0.
    • Update 5.0 Migration Guide about the change

Where Should the Reviewer Start?

I would probably start with Box: modules/labs-react/common/lib/Box.tsx. And because most of the component's logic lives in the style props, I would look at: modules/labs-react/common/lib/utils. From there, I would hop over to Flex: modules/labs-react/layout/lib/Flex.tsx and then to Stack: modules/labs-react/layout/lib/Stack.tsx.

Testing Manually

  • Box tests
    • Unit tests: yarn test modules/labs-react/common/spec
    • Visual Regression Tests: visit http://localhost:9001/?path=/story/testing-react-labs-common--box-states
  • Flex and Stack tests:
    • Unit tests: yarn test modules/labs-react/layout/spec
    • Visual Regression Tests:
      • http://localhost:9001/?path=/story/testing-react-labs-layout-flex
      • http://localhost:9001/?path=/story/testing-react-labs-layout-stack

Screenshots

Box

Screen Shot 2021-05-06 at 8 41 17 PM

Flex

Screen Shot 2021-05-06 at 8 43 12 PM

Stack

Screen Shot 2021-05-06 at 8 43 20 PM

Thank You Gif

These components are small and simple, but I'm excited to see what you'll build with them. 😄

Checklist

  • branch has been rebased on the latest master commit
  • tests are changed or added
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

@alanbsmith alanbsmith changed the base branch from master to prerelease/v5 April 16, 2021 17:37
@cypress
Copy link

cypress bot commented Apr 19, 2021



Test summary

613 0 0 0


Run details

Project canvas-kit
Status Passed
Commit 4de335e ℹ️
Started May 25, 2021 6:12 PM
Ended May 25, 2021 6:19 PM
Duration 07:30 💡
OS Linux Ubuntu Linux - 20.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

Copy link
Member

@jamesfan jamesfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanbsmith I did a preliminary review of the docs, I think they look pretty good overall 👍. Just a few minor thoughts below.

modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
@alanbsmith alanbsmith force-pushed the create-layout-components branch 2 times, most recently from 9156276 to 709d182 Compare May 3, 2021 18:00
.yarnrc Outdated Show resolved Hide resolved
.yarnrc Outdated Show resolved Hide resolved
@NicholasBoll
Copy link
Member

The Typescript error is strange:

/home/runner/work/canvas-kit/canvas-kit/node_modules/typescript/lib/typescript.js:11005
        return node.kind === 75 /* Identifier */ || node.kind === 76 /* PrivateIdentifier */;
                    ^

TypeError: Cannot read property 'kind' of undefined
    at Object.isIdentifierOrPrivateIdentifier (/home/runner/work/canvas-kit/canvas-kit/node_modules/typescript/lib/typescript.js:11005:21)
    at Object.getTextOfIdentifierOrLiteral (/home/runner/work/canvas-kit/canvas-kit/node_modules/typescript/lib/typescript.js:15467:19)
    at createLiteralFromNode (/home/runner/work/canvas-kit/canvas-kit/node_modules/typescript/lib/typescript.js:68198:43)
    at Object.createLiteral (/home/runner/work/canvas-kit/canvas-kit/node_modules/typescript/lib/typescript.js:68168:16)
    at setType (/home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:199:83)
    at createPropDefinition (/home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:206:9)
    at /home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:90:20
    at Array.map (<anonymous>)
    at setComponentDocGen (/home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:88:165)
    at /home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:35:13
    at Array.map (<anonymous>)
    at Object.generateDocgenCodeBlock [as default] (/home/runner/work/canvas-kit/canvas-kit/node_modules/react-docgen-typescript-loader/dist/generateDocgenCodeBlock.js:32:44)
    at processModule (/home/runner/work/canvas-kit/canvas-kit/.storybook/docgen-plugin.js:54:15)
    at modulesToProcess.forEach.m (/home/runner/work/canvas-kit/canvas-kit/.storybook/docgen-plugin.js:35:39)
    at Array.forEach (<anonymous>)
    at compilation.hooks.seal.tap.modules (/home/runner/work/canvas-kit/canvas-kit/.storybook/docgen-plugin.js:35:26)

That looks like something that is confusing React Docgen

@jpante jpante linked an issue May 4, 2021 that may be closed by this pull request
Copy link
Contributor

@anicholls anicholls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to delete the space function in labs-tokens.

modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/lib/utils/space.ts Outdated Show resolved Hide resolved
modules/labs-react/layout/README.md Outdated Show resolved Hide resolved
modules/react/tokens/lib/radius.ts Outdated Show resolved Hide resolved
modules/labs-react/layout/stories/Flex.stories.mdx Outdated Show resolved Hide resolved

#### Using `shouldWrapChildren`

Because of the increased specificity of `Stack`'s pseduo-classes, it may override the child element
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this description isn't clear enough to indicate exactly when you should or shouldn't use this prop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I need to rethink this content a bit. I feel like there should be some general information below Managing Child Elements and then explain how each option works in these subsections.

modules/labs-react/common/lib/utils/border.ts Show resolved Hide resolved
@alanbsmith alanbsmith changed the title Create layout components [ISSUE-771] Add Box, Flex, and Stack May 7, 2021
@alanbsmith alanbsmith marked this pull request as ready for review May 7, 2021 06:13
@alanbsmith alanbsmith added the ready for review Code is ready for review label May 7, 2021
Copy link
Member

@jamesfan jamesfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanbsmith Awesome work on the Box docs! What a beast -- I think you've done about as well as you could have to organize it. This had to have been an incredible amount of work to put together. 👏

I left a few suggestions below. Didn't notice any major issues with the content, mostly just nits and spelling errors.

modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
modules/labs-react/common/stories/Box.stories.mdx Outdated Show resolved Hide resolved
alanbsmith and others added 11 commits May 21, 2021 09:20
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
Co-authored-by: James Fan <james@somewhatstrange.com>
@alanbsmith alanbsmith merged commit 68e0c32 into Workday:prerelease/v5 May 25, 2021
@NicholasBoll NicholasBoll mentioned this pull request Jul 7, 2022
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x ready for review Code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Box, Flex, and Stack
4 participants