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

Added top, left, right and bottom border color CSS properties to system #27580

Merged
merged 4 commits into from
Aug 6, 2021

Conversation

R-Bower
Copy link
Contributor

@R-Bower R-Bower commented Aug 2, 2021

Related issue

The problem:

  • When specifying borderColor using @material-ui/system, values aren't properly applied if the borderColor prop appears before the border<Position> prop.

Example:

// the color for border-top is `#000000` in this example
<Box borderColor="green" borderTop={1} />

Existing workarounds:

// we have to specify every property 
<Box
  border={1}
  borderColor="background.default"
  borderBottom={0}
  borderLeft={0}
  borderRight={0}
/>
// *OR* ensure that `borderColor` is ordered after the target border property
<Box
  borderTop={1}
  borderColor="background.default"
/>

Changes in the PR:

  • Added borderTopColor, borderRightColor, borderBottomColor, and borderLeftColor CSS properties to @material-ui/system borders.
  • Updated markdown tables in border docs to reflect the addition of these properties.

With this PR, the previous workaround is no longer necessary. Instead, users can specify the exact border<Position>Color property, like so:

<Box
  borderTop={1}
  borderTopColor="background.default"
 />

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 2, 2021

Details of bundle changes (experimental)

@material-ui/system: parsed: +0.87% , gzip: +0.30%

Generated by 🚫 dangerJS against 8164daa

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Changes look good, let’s also add tests that will ensure this new scenario would break again. Also, please revert the changes in the translated md files. Only the changes in borders.md are enough

@R-Bower
Copy link
Contributor Author

R-Bower commented Aug 2, 2021

Changes look good, let’s also add tests that will ensure this new scenario would break again. Also, please revert the changes in the translated md files. Only the changes in borders.md are enough

Sounds good. Where do you recommend I add these tests? I think it'd need to involve some mock rendering. The existing borders.test.js only tests the borderRadius conversion.

@mnajdova
Copy link
Member

mnajdova commented Aug 2, 2021

We’ve added the other border tests in https://github.com/mui-org/material-ui/blob/next/packages/material-ui-system/src/Box/Box.test.js I propose we add these there too.

@R-Bower
Copy link
Contributor Author

R-Bower commented Aug 3, 2021

@mnajdova I've added tests and they've passed, but test_browser-1 is failing on ci/circleci. I just checked some other recent PRs and the same test is failing for those as well.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Thanks! 👍

@mnajdova mnajdova merged commit 829310c into mui:next Aug 6, 2021
@mnajdova mnajdova changed the title Added top/right/bottom/left border-color CSS properties to system Added top, left, right and bottom border color CSS properties to system Aug 6, 2021
@zannager zannager added the package: system Specific to @mui/system label Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants