adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.96k stars 1.13k forks source link

mergeProps should combine styles #2507

Open tinleym opened 3 years ago

tinleym commented 3 years ago

🙋 Feature Request

mergeProps should return combined styles, rather than only returning the last.

🤔 Expected Behavior

mergeProps({style: {height: 50px, width: 50px}}, {style: {height: 75px, color: orange}}) ===> {style: {height: 75px, width: 50px, color: orange}}

😯 Current Behavior

mergeProps({style: {height: 50px, width: 50px}}, {style: {height: 75px, color: orange}}) ===> {style: {height: 75px, color: orange}}

🔦 Context

Some react-aria hooks, like useOverlayPosition, return a style object.

I realize there are a lot of other (arguably better) ways to approach this, like keeping useOverlayPosition's style output out of mergeProps altogether, but all things being equal, it would be nice to be if there was a more ergonomic way to combine those styles.

💻 Examples

So, instead of this... <div {...mergeProps(overlayTriggerProps, {style: {...overlayPositionProps.style, ...myStyles }})} />

This... <div {...mergeProps(overlayTriggerProps, overlayPositionProps, myStyles)} />

LFDanLu commented 3 years ago

This sounds like a decent idea, lemme check if there were any reason we hadn't done this in the first place

LFDanLu commented 3 years ago

Basic style object merging is something we can add to mergeProps but it's worth mentioning that there are some complicated cases that would be hard to handle (e.g. merging a css property that supports multiple values)

snowystinger commented 3 years ago

some examples of difficult ones https://jsfiddle.net/h3yzsbqj/1/ which is why we'd lean towards just plain merging with overwrite

tinleym commented 3 years ago

Makes sense, thanks for taking a look!

snowystinger commented 3 years ago

sorry for the confusion, i meant plain merge two style objects with overwrite essentially {...stylePropsA, ...stylePropsB} and not try to get fancy with things like this

let stylePropsA = {style: {
  background: center / contain no-repeat url("https://interactive-examples.mdn.mozilla.net/media/examples/firefox-logo.svg");
}};
let stylePropsB = {style: {
  background: #eee 35% url("https://interactive-examples.mdn.mozilla.net/media/examples/lizard.png");
}};
let result = mergeProps(stylePropsA, stylePropsB);
// result === {style: {
// background: center / contain no-repeat url("https://interactive-examples.mdn.mozilla.net/media/examples/firefox-// logo.svg"),
//             #eee 35% url("https://interactive-examples.mdn.mozilla.net/media/examples/lizard.png");
// }}

instead, background would just be stylePropsB's background