Skip to content

Conversation

@BinaryMuse
Copy link
Contributor

@BinaryMuse BinaryMuse commented Apr 14, 2020

As discussed in #510, this PR will be adjusting BorderBox and related components as follows:

  • Box will receive FLEX props

  • Flex will be display: flex by default but otherwise is identical to Box

  • BorderBox will have its border default prop split into borderWidth and borderStyle.

    This is a breaking change that will have ramifications for many of our own components and also for Doctocat; a migration strategy will be included.

  • The sx prop will be made available to all components

    The implementation of the sx prop is still in progress, although we seem to have a path forward. I also want to ensure we have good documentation around the prop, especially around when it should and should not be used.

    Moving this task to its own PR

Closes #510

Doctocat shadowing

A number of components in Doctocat use shorthand border properties to modify the border on BorderBox (as shown in the release notes section below). These components have been shadowed in this PR; once it ships, the Doctocat components can be changed and these shadowing files can be removed.

Release Notes

  • BorderBox has had its default border prop split into borderWidth and borderStyle:

    before:

    BorderBox.defaultProps = {
      border: '1px solid',
      borderColor: 'gray.2',
      borderRadius: 2
    }

    after:

    BorderBox.defaultProps = {
      borderWidth: '1px',
      borderStyle: 'solid',
      borderColor: 'gray.2',
      borderRadius: 2
    }

    While this change was made to fix bugs in the ability to modify BorderBox's border, it is also a breaking change if you use border or border<Direction> shorthand props to modify the border, as these will no longer work. Instead, use non-shorthand props. For example, change

    <BorderBox border={0} borderTop={1}>...</BorderBox>

    to

    <BorderBox borderWidth={0} borderTopWidth={1}>...</BorderBox>
  • Box (and components that inherit from it, like BorderBox) now have access to FLEX system props.

  • Flex now behaves exactly like Box except that it has its display prop set to flex by default.

  • Flex.Item has been removed in favor of Box, which now has all the same props.

  • theme.borders has been removed; use the new borderWidths theme values for borderWidth properties (0 is a width of 0, 1 is a width of 1px).

  • The following colors have been added:

    • border.blackFade
    • border.blue
    • border.blueLight
    • border.grayDarker
    • border.green
    • border.greenLight
    • border.purple
    • border.red
    • border.redLight
    • border.white
    • border.whiteFade
    • border.yellow

Merge checklist

  • Added or updated TypeScript definitions (index.d.ts) if necessary
  • Added/updated tests
  • Added/updated documentation
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@vercel
Copy link

vercel bot commented Apr 14, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-components/jsen2oaf0
✅ Preview: https://primer-components-git-mkt-nobody-puts-border-in-a-box.primer.now.sh

@vercel vercel bot temporarily deployed to Preview April 16, 2020 18:48 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2020 18:54 Inactive
@BinaryMuse BinaryMuse force-pushed the mkt/nobody-puts-border-in-a-box branch from 4fb1397 to d6dba25 Compare April 16, 2020 18:56
@BinaryMuse BinaryMuse changed the base branch from master to release-19.0.0 April 16, 2020 18:56
@vercel vercel bot temporarily deployed to Preview April 16, 2020 18:56 Inactive
@vercel vercel bot temporarily deployed to Preview April 16, 2020 19:01 Inactive
@BinaryMuse BinaryMuse marked this pull request as ready for review April 16, 2020 19:04
@BinaryMuse BinaryMuse requested a review from emplums April 16, 2020 19:04
@BinaryMuse BinaryMuse changed the title BorderBox and related changes BorderBox and related (breaking) changes Apr 16, 2020
@BinaryMuse
Copy link
Contributor Author

This should be ready for review.

/cc @emplums
/cc @colebemis for any issues related to Doctocat shadowing
/cc @colebemis, @dmarcey, @smockle off the top of my head for the TypeScript typings changes. I'm not 100% confident in these.

@emplums
Copy link

emplums commented Apr 17, 2020

One thing I noticed while going through the type definitions - BorderBox has shadow props, but Box doesn't. Does that feel right? I feel like Box should probably have shadow props too 🤔

@BinaryMuse
Copy link
Contributor Author

BorderBox has shadow props, but Box doesn't. Does that feel right?

@emplums Currently we add styledSystem.shadow system props only to BORDER.

export const BORDER = compose(styledSystem.border, styledSystem.shadow)
BORDER.propTypes = {
  ...systemPropTypes.border,
  ...systemPropTypes.shadow
}

I could see a theoretical argument that borders and shadows are similarish, but don't have strong opinions. Would we move it to COMMON?

@vercel vercel bot temporarily deployed to Preview April 17, 2020 18:59 Inactive
@vercel vercel bot temporarily deployed to Preview April 17, 2020 19:12 Inactive
@vercel vercel bot temporarily deployed to Preview April 17, 2020 19:18 Inactive
@vercel vercel bot temporarily deployed to Preview April 17, 2020 20:13 Inactive
@emplums
Copy link

emplums commented Apr 20, 2020

@BinaryMuse on second thought, I think it's fine to just leave in BorderBox, but could you add an example to the BorderBox docs showing box shadow usage?

Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

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

Left a few more comments but once those are resolved I think this is good to go!

@vercel vercel bot temporarily deployed to Preview April 20, 2020 21:39 Inactive
@vercel vercel bot temporarily deployed to Preview April 20, 2020 21:46 Inactive
@BinaryMuse
Copy link
Contributor Author

I added all the border colors from Primer CSS, including the *Fade ones:

image

I also pulled in polished so we can stop hardcoding mysterious hex and rgba values all over the place.

-    grayLight: '#eaecef',
+    grayLight: lighten(0.03, gray[2]),

@BinaryMuse BinaryMuse merged commit 864cf54 into release-19.0.0 Apr 20, 2020
@BinaryMuse BinaryMuse deleted the mkt/nobody-puts-border-in-a-box branch April 20, 2020 21:57
This was referenced Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🌀[API] Adjust BorderBox Props

3 participants