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

feat(components): add Box primitive to components library #5665

Merged
merged 1 commit into from
May 13, 2020

Conversation

mcous
Copy link
Contributor

@mcous mcous commented May 12, 2020

overview

Palate cleanser PR for myself after the U2E project is officially dev complete. This PR follows up on #5637 by:

  • Adding a <Box> primitive to the library
    • Just a <div> with styling props to reduce CSS repetition
  • Adding spacing props to the primitive components
    • They allow you to set margin and padding
    • They inlcude very handy marginX, marginY, paddingX, and paddingY props to set left/right or top/bottom simultaneously
    • Also added a simple set of power-of-two space scale constants - the props don't require using the constants, though; they'll take whatever string or number you give them
    • They also make it very easy to ensure that only parent components deal with margins, which makes child components easier to deal with

These components and props are, like #5637, pretty heavily inspired by theme-ui, styled-system, and rebass.

changelog

  • feat(components): add Box primitive to components library
    • Took the opportunity to address some TODO comments and consolidate some style prop logic into shareable chunks in components/src/primitives/style-props.js

review requests

How does this look / feel? I replaced some inline styled-component styling logic with the new Box primitive and spacing props to demonstrate.

A really nice benefit of these primitives and styling props is that the more we can lean on them, the less reliant we are on any one particular styling solution. These primitives / props could be implemented in vanilla CSS, CSS Modules, styled-components, emotion, or whatever other thing without changing their APIs

risk assessment

Low. Mostly internal components DX work, and logic is unit-tested all the way down to the CSS

@mcous mcous added feature Ticket is a feature request / PR introduces a feature ready for review components Affects the `components` project labels May 12, 2020
@mcous mcous requested a review from a team as a code owner May 12, 2020 22:33
@mcous mcous requested review from b-cooper and removed request for a team May 12, 2020 22:33
@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #5665 into edge will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             edge    #5665   +/-   ##
=======================================
  Coverage   65.11%   65.12%           
=======================================
  Files        1112     1114    +2     
  Lines       30802    30809    +7     
=======================================
+ Hits        20056    20063    +7     
  Misses      10746    10746           
Impacted Files Coverage Δ
...pp/src/components/SystemInfoCard/U2EAdapterInfo.js 100.00% <ø> (ø)
components/src/primitives/Flex.js 100.00% <ø> (ø)
components/src/primitives/Link.js 100.00% <ø> (ø)
components/src/primitives/Text.js 100.00% <ø> (ø)
components/src/structure/Card.js 100.00% <ø> (ø)
.../src/components/SystemInfoCard/U2EDeviceDetails.js 100.00% <100.00%> (ø)
components/src/primitives/Box.js 100.00% <100.00%> (ø)
components/src/primitives/style-props.js 100.00% <100.00%> (ø)
components/src/styles/spacing.js 100.00% <100.00%> (ø)
components/src/styles/typography.js 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02cd054...b0a97b6. Read the comment docs.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Everything looks good. The only one I'm a little skeptical of is the SPACING_FOO constants. I think that it's wishful thinking to imagine that the spacing in a given design won't fall somewhere between say SPACING_4 and SPACING_5. In which case, we have to hard code a rem value, and there's no good way, aside from rote memorization to remember whether 10rem is more or less than SPACING_5.

@mcous mcous merged commit 73614d0 into edge May 13, 2020
@mcous mcous deleted the components_primitive-common-props branch May 13, 2020 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
components Affects the `components` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants