Repository navigation
Warn when using overlapping styles (e.g. border and borderBottom) #6348
Description
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.
IMHO, all things considered it's better to just disallow overlapping and warn in dev.
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:
- marginBottom gets dropped sometimes #2231
- Adding and then removing "background" from style attribute creates unwanted styles #2407
- CSS
backgroundSizeproperty lost when re-rendering an image URL inside of an adjacentbackgroundproperty #5030 - Decompose shorthand/compound styles to their constituent parts FormidableLabs/radium#95
Related PRs:
Activity
mattborn commentedon 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 notborderBottom. 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 commentedon Mar 28, 2016
Not sure I understand the question, can you elaborate?
mattborn commentedon Mar 29, 2016
Sure. If it is invalid to have both
borderandborderColoron 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 shorthandborderattribute 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 shorthandborderis still technically valid. However, if there is an attribute in a separate style object—like changingborderColoron hover for example—that would affect the border involved and will get applied at runtime to the node that already has shorthandborderon it, I’d like to be warned as early as possible as this is where things tend to break for me.gaearon commentedon 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 commentedon 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 commentedon Mar 29, 2016
What about situations where a third-party component defines styles and allows overrides? Say a component defines its own
borderstyle and I want to just override theborderBottomstyle?rtsao commentedon 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)
[core] Apply 3 different fixes (#5695)
sophiebits commentedon 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 commentedon 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 commentedon May 24, 2018
@sophiebits, I am happy to work on this one :)
11 remaining items
sophiebits commentedon Oct 9, 2018
All right @YongPilMoon, it's yours.
sophiebits commentedon Oct 9, 2018
Adding a note here: removing a property can also trigger this bug. https://stackoverflow.com/q/52636618/49485
Add warning about css shorthand properties collision(react#6348)
Warn about conflicting style values during updates
Warn about conflicting style values during updates
Warn about conflicting style values during updates (#14181)
Warn about conflicting style values during updates (react#14181)
Warn about conflicting style values during updates (react#14181)
gaearon commentedon Feb 26, 2020
The warning is out in 16.13.
https://reactjs.org/blog/2020/03/02/react-v16.13.0.html
[core] Apply 3 different fixes (#5695)
[core] Apply 3 different fixes (mui#5695)
Warn about conflicting style values during updates (react#14181)