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

Warn when using overlapping styles (e.g. border and borderBottom) #6348

Closed
gaearon opened this issue Mar 26, 2016 · 20 comments
Closed

Warn when using overlapping styles (e.g. border and borderBottom) #6348

gaearon opened this issue Mar 26, 2016 · 20 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Mar 26, 2016

This "good first bug" is taken by @YongPilMoon. Don't work on it unless that's you!

This has been discussed a few times before but I don’t think there was any conclusion, and PRs intending to solve the issue were not merged for various reasons.

I would like to close those PRs as outdated, and reboot the discussion about this.
Performance considered, the conclusion from @sebmarkbage and @syranide seems to be:

We should consider not allowing conflicting style rules at all.

#2013 (comment)

IMHO, all things considered it's better to just disallow overlapping and warn in dev.

#4661 (comment)

Radium by @ianobermiller came to the same conclusion in FormidableLabs/radium#95 but there was some backlash afterwards. React Native seems to allow style expansions but only for a few attributes (e.g. margin and padding, but not border).

I’m closing old pull requests about this, and creating this issue to track implementation of the behavior we seem to agree upon: we should warn in __DEV__ when border and borderBottom are used at the same time. We can discuss more specifics (should either be ignored? should we allow a few whitelisted properties but warn for others?) in this issue.

As a migration strategy, we can suggest people to use something like https://github.com/ActionIQ/style-builder if they really need those shortcuts. It’s also something we’ll need to decide upon when implementing integrated styling.


Related issues:

Related PRs:

@mattborn
Copy link

@mattborn mattborn commented Mar 28, 2016

@gaearon Thank you for resurfacing this! Crazy that it has been a year since I opened the Radium issue.

I like the idea of warning about shorthand properties like border, but not borderBottom. There shouldn’t be any surprises when merging style objects using the longer property names.

Question. Is the intent of this issue to address only the analysis of individual style objects or is there also a desire to analyze and warn about possible collisions of properties that are conditionally applied?

@gaearon
Copy link
Member Author

@gaearon gaearon commented Mar 28, 2016

Question. Is the intent of this issue to address only the analysis of individual style objects or is there also a desire to analyze and warn about possible collisions of properties that are conditionally applied?

Not sure I understand the question, can you elaborate?

@mattborn
Copy link

@mattborn mattborn commented Mar 29, 2016

Sure. If it is invalid to have both border and borderColor on the same element, then it makes sense to warn about this. In my experience, this combination is easy to avoid, but it is more difficult to anticipate and debug when styles are being applied dynamically (conditional logic based on props or state, media queries, Radium, etc.) or passed down as props and merged with lower order style objects. If a warning for the shorthand border attribute occurs when there are no other attributes that might affect the border involved, then the warning seems more like a dogmatic rule than a helpful debugging tool since shorthand border is still technically valid. However, if there is an attribute in a separate style object—like changing borderColor on hover for example—that would affect the border involved and will get applied at runtime to the node that already has shorthand border on it, I’d like to be warned as early as possible as this is where things tend to break for me.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Mar 29, 2016

Ah, good point. I’d probably warn only if we know the styles overlap but I’m not sure how easy it is to tell. I haven’t thought about it much and I don’t have a good solution.

@jimfb
Copy link
Contributor

@jimfb jimfb commented Mar 29, 2016

@gaearon An alternate solution that I like better is to use csstext as per #5397, which IIRC cleanly solves a superset of these issues.

@aweary
Copy link
Collaborator

@aweary aweary commented Mar 29, 2016

What about situations where a third-party component defines styles and allows overrides? Say a component defines its own border style and I want to just override the borderBottom style?

@rtsao
Copy link

@rtsao rtsao commented Sep 13, 2016

I've started working on dead-simple helper functions to convert shorthand properties to longform (among other things): https://github.com/rtsao/lostyle/tree/master/src (docs)

oliviertassinari added a commit to mui-org/material-ui that referenced this issue Dec 1, 2016
- Prevent using style conflicting properties like border and border Button
facebook/react#6348
- Avoid using ES6 features that can't be transpiled like includes
- Remove dead style properties
@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented May 24, 2018

This is super unpredictable and confusing and I just ran into this.

I'd suggest we at least warn on updates when we know it's problematic? That is, if we go from {background: a, backgroundSize: b} to {background: c, backgroundSize: b}, warn that backgroundSize is being overridden (but if backgroundSize changes at the same time, don't warn). We can do this for all property shorthands and know whether it's problematic from the property enumeration order.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented May 24, 2018

Marking as "good first issue". Let's add a DEV-only warning for this per my comment above. I think a good place to begin investigating would be here:

https://github.com/facebook/react/blob/v16.4.0/packages/react-dom/src/client/ReactDOMFiberComponent.js#L619

If you'd like to take this on, please comment here so that we don't have two people working on this.

@supertinou
Copy link

@supertinou supertinou commented May 24, 2018

@sophiebits, I am happy to work on this one :)

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented May 24, 2018

@supertinou It's yours!

@craigkovatch
Copy link

@craigkovatch craigkovatch commented Jun 13, 2018

Just hit this and was indeed surprised and confused by it.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Jun 13, 2018

@supertinou Just wanted to check if there's anything I can help you with on this.

@mirkojotic
Copy link

@mirkojotic mirkojotic commented Jun 18, 2018

@sophiebits Hi I don't want to step on anybodies toes :D but I've taken a look into this issue, replicated it and would love to take a stab at it but I don't know if @supertinou is still working on it.

@giuseppeg
Copy link
Contributor

@giuseppeg giuseppeg commented Aug 3, 2018

I made a codepen to recap/explain what is going on https://codepen.io/giuseppegurgone/pen/mjKbmq?editors=0010 @gaearon / @sophiebits let me know if I forgot something.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Sep 27, 2018

@mirkojotic Still interested?

@YongPilMoon
Copy link

@YongPilMoon YongPilMoon commented Sep 28, 2018

If there is no one i would like to take this one :)

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Oct 9, 2018

All right @YongPilMoon, it's yours.

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented Oct 9, 2018

Adding a note here: removing a property can also trigger this bug. https://stackoverflow.com/q/52636618/49485

sophiebits added a commit to sophiebits/react that referenced this issue Nov 9, 2018
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
sophiebits added a commit to sophiebits/react that referenced this issue Nov 9, 2018
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
sophiebits added a commit that referenced this issue Nov 9, 2018
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes #6348.
jetoneza added a commit to jetoneza/react that referenced this issue Jan 23, 2019
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
n8schloss added a commit to n8schloss/react that referenced this issue Jan 31, 2019
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
@gaearon
Copy link
Member Author

@gaearon gaearon commented Feb 26, 2020

The warning is out in 16.13.
https://reactjs.org/blog/2020/03/02/react-v16.13.0.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.