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

Using borderColor results in !important being added to the style rule #16853

Closed
2 tasks done
rbrtsmith opened this issue Aug 2, 2019 · 6 comments · Fixed by #16875
Closed
2 tasks done

Using borderColor results in !important being added to the style rule #16853

rbrtsmith opened this issue Aug 2, 2019 · 6 comments · Fixed by #16875
Labels
good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request package: system Specific to @mui/system

Comments

@rbrtsmith
Copy link

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

I'd expect the style rule to be applied without !important.

Current Behavior 😯

!important gets added to the style rule.

Steps to Reproduce 🕹

Link:

  1. Import MUI Box
  2. Render with borderColor property: <Box borderColor="red" />
  3. https://codesandbox.io/s/create-react-app-with-typescript-huvb9

Context 🔦

I wasn't sure if this was a bug or if it's intentional but it feels like adding !important to border style rules is a bit heavy handed and could cause potential specificity issues for example an A/B test from Optimizely attempting to override some style rules.
If this behaviour is intentional it would be good to get an understanding of the intent behind it.

Please see the screenshot below taken from the Codesandbox I linked.
Screen Shot 2019-08-02 at 09 06 58

The source of the !important being added

Your Environment 🌎

Tech Version
Material-UI v4.3.2
React
Browser
TypeScript
etc.
@oliviertassinari
Copy link
Member

@rbrtsmith The !important was added on purpose to make this demo work: https://material-ui.com/system/borders/#border-color. However, it seems that we could remove it with the folllowing approach:

diff --git a/docs/src/pages/system/borders/BorderColor.js b/docs/src/pages/system/borders/BorderColor.js
index 2f057fef6..d16cc7afc 100644
--- a/docs/src/pages/system/borders/BorderColor.js
+++ b/docs/src/pages/system/borders/BorderColor.js
@@ -2,28 +2,21 @@ import React from 'react';
 import Grid from '@material-ui/core/Grid';
 import Box from '@material-ui/core/Box';

-const inner = (
-  <Box bgcolor="background.paper" m={1} border={1} style={{ width: '5rem', height: '5rem' }} />
-);
+const defaultProps = {
+  bgcolor: 'background.paper',
+  m: 1,
+  border: 1,
+  style: { width: '5rem', height: '5rem' },
+};

 function BorderColor() {
   return (
     <Grid container justify="center">
-      <Box borderColor="primary.main" clone>
-        {inner}
-      </Box>
-      <Box borderColor="secondary.main" clone>
-        {inner}
-      </Box>
-      <Box borderColor="error.main" clone>
-        {inner}
-      </Box>
-      <Box borderColor="grey.500" clone>
-        {inner}
-      </Box>
-      <Box borderColor="text.primary" clone>
-        {inner}
-      </Box>
+      <Box borderColor="primary.main" {...defaultProps} />
+      <Box borderColor="secondary.main" {...defaultProps} />
+      <Box borderColor="error.main" {...defaultProps} />
+      <Box borderColor="grey.500" {...defaultProps} />
+      <Box borderColor="text.primary" {...defaultProps} />
     </Grid>
   );
 }
diff --git a/packages/material-ui-system/src/borders.js b/packages/material-ui-system/src/borders.js
index 5354bad92..65c3c6f27 100644
--- a/packages/material-ui-system/src/borders.js
+++ b/packages/material-ui-system/src/borders.js
@@ -6,7 +6,7 @@ function getBorder(value) {
     return value;
   }

-  return `${value}px solid${value === 0 ? ' !important' : ''}`;
+  return `${value}px solid`;
 }

 export const border = style({
@@ -42,7 +42,6 @@ export const borderLeft = style({
 export const borderColor = style({
   prop: 'borderColor',
   themeKey: 'palette',
-  transform: value => `${value} !important`,
 });

 export const borderRadius = style({

What do you think?

@oliviertassinari oliviertassinari added new feature New feature or request package: system Specific to @mui/system labels Aug 2, 2019
@rbrtsmith
Copy link
Author

@oliviertassinari Thanks for looking at this and explaining the intent. I would be in favour of your suggested approach. That would fix the problem on our end 😄

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 2, 2019

@rbrtsmith Ok, I think that it's worth doing. Do you want to submit a pull request? :)

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Aug 2, 2019
@rogerclotet
Copy link
Contributor

I went ahead and submitted a PR with your suggestion. I was looking for an easy issue to submit my first pull request and this one seemed appropriate. I hope that's okay.

I still have to figure out the workflow to develop more complex changes in local, I followed the contribution guide but I'm having some weird issues with yarn link and had to test it in a more manual way.

@mbrookes
Copy link
Member

mbrookes commented Aug 4, 2019

@rogerclotet The easiest way to test things locally is to fire up the docs site with yarn && yarn start

@rogerclotet
Copy link
Contributor

Yeah, tried that as well, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants