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

Compound variants #80

Closed
peduarte opened this issue Aug 10, 2020 · 17 comments
Closed

Compound variants #80

peduarte opened this issue Aug 10, 2020 · 17 comments
Labels
feature New feature

Comments

@peduarte
Copy link
Contributor

We need to have a think about how to support compound variants. This mean the ability to define styles when two or more variants are applied.

For example, a Button with the following variants

  • color
    • blue
    • red
  • appearance
    • normal
    • ghost

When the appearance="ghost", we want to set a backgroundColor. This color should be based on the color variant. In plain CSS:

button.ghost.blue:hover {
  backgroundColor: 'blue'
}
button.ghost.red:hover {
  backgroundColor: 'red'
}

The usage for to represent this in React, would be:

<Button color="red" appearance="ghost" />

Let's start a conversation around how to support this and how the API should be 🙌

@peduarte
Copy link
Contributor Author

Initial API idea from Colm

styled('button', {}, {
  color: {
    blue: {}
    red: {},
  },
  appearance: {
    normal: {},
    ghost: {
      backgroundColor: 'transparent',
      'color.blue': {
        backgroundColor: 'blue',
      },      
      'color.red': {
        backgroundColor: 'red',
      },      
    }
  }
})

@colmtuite

@christianalfoni
Copy link
Contributor

christianalfoni commented Aug 17, 2020

Only reason I come up with an other suggestion here is Typescript 😄

styled('button', {}, {
  color: {
    blue: {}
    red: {},
  },
  appearance: {
    normal: {},
    ghost: [{
      css: { backgroundColor: 'transparent' }
    }, {
      variants: { color: 'blue' },
      css: { backgroundColor: 'blue' },
    }, {
      variants: { color: 'red' },
      css: { backgroundColor: 'red' },
    }]
  }
})

@peduarte
Copy link
Contributor Author

peduarte commented Aug 17, 2020

Ah, I see.

I like your suggestion. It basically follows the same structure as creating a variant in the first place, so nothing new to learn 😄

Syntax wise is not as simple, but, its worth noting that this isn't the most common thing to do either.

@colmtuite you cool with this?

@colmtuite
Copy link

Hmmm, seems way too complicated to me. I think I know how it works, but it's not intuitive at all to me. I'm expecting to just be able to nest one variant inside another.

@peduarte
Copy link
Contributor Author

Hmm yeah. if I think from a Typescript perspective, I like it. But then back to DX, its not the most intuitive approach, its true.

@christianalfoni is the proposed approach even possible? Trying to understand what are our options

@christianalfoni
Copy link
Contributor

christianalfoni commented Aug 17, 2020

Yeah, I totally see the syntax argument here. Only reason I suggest it is because the first suggestion will not work with Typescript :-/

One could also argue in the initial suggestions that it looks like a selector. So, the user might try:

{
    ghost: {
      backgroundColor: 'transparent',
      'color.blue': {
        backgroundColor: 'blue',
      },      
      ':hover': {
        'color.red': {
          backgroundColor: 'red',
        }
      },      
    }
}

...which is a bit weird and difficult to take into account as we would have to "hoist" all these nested variant references to the top.

Also from a technical standpoint it is very tricky to implement, because they are like "inside" the CSS definition itself, handled by the css package. But it only relates to the variants of the styled package. So we would kinda have to go in there and extract them.

But lets try to iterate a bit more, maybe we will find something 😄 So just to throw something out for inspiration:

styled('button', {}, {
  color: {
    blue: {}
    red: {},
  },
  appearance: {
    normal: {},
    ghost: {
      backgroundColor: 'transparent',
    }
  }
}, {
  color: {
    blue: {
      appearance: {
        ghost: { backgroundColor: 'blue' }
      }
    },
    red: {
      appearance: {
        ghost: { backgroundColor: 'red' },
      }
    }
  }
})

Totally see a lot of nesting here, but it does add an explicit concept of compound variants. So it describes it pretty well. You would also be able to express "How does the color variant blue affect the other variants?":

 {
  color: {
    blue: {
      appearance: {
        normal: {},
        ghost: { backgroundColor: 'blue' }
      },
     someOtherVariant: { ... }
    },
  }
}

I totally see the first suggestion wins on syntax, but it does have some technical implications and also the typing. But yeah... gotten myself a frozen pizza here (that is how I treat myself) and lets see if some other ideas pop up as well? 😄

@peduarte
Copy link
Contributor Author

peduarte commented Aug 18, 2020

Hmm, thinking a bit:

Could we have some helper function?

{
  color: {
    red: {}
  },
  appearance: {
    ghost: {
       ...withVariant('color.red', {
          backgroundColor: 'red'
       })
    } 
  } 
}

Or, maybe, just maybe... we get a function callback with the variants? Not sure on perf impacts

styled('button', {}, variants => ({
  color: {
    red: {}
  },
  appearance: {
    ghost: {
       [variants.color.red]: {
          backgroundColor: 'red'
       })
    } 
  } 
}))

⚠️ ⚠️ ⚠️ Brainstorming ⚠️ ⚠️ ⚠️

If we did move variants to its own function call, like issue suggests: #85 (comment)

Button.variant('color', { red: {  } })
Button.variant('appearance', { ghost: {  } })

Button.compoundVariant('color', 'appearance', { 
  'red.ghost': { backgroundColor: 'red' }
})

Or

Button.variant('color', { red: {  } })
Button.variant('appearance', { ghost: {  } })

Button.compoundVariant({
    appearance: 'ghost',
    color: 'red'
  },
  {
    bg: 'red'
})

@hadihallak
Copy link
Member

hadihallak commented Aug 18, 2020

@peduarte if we could get this to work with ts

Button.variant('color', { red: {  } })
Button.variant('appearance', { ghost: {  } })

Button.compoundVariant({
    appearance: 'ghost',
    color: 'red'
  },
  {
    bg: 'red'
})

that would be the best-case scenario for me.

@colmtuite
Copy link

colmtuite commented Aug 18, 2020

Still waiting for my jury to return on the compoundVariant thingy, but I'm all for moving variants into their own function call 💯

Agree with Abdul on preferring version 2.

@peduarte
Copy link
Contributor Author

@colmtuite Ok, cool. Looks like we're getting somewhere, if @hadihallak and yourself are happy with the 2nd proposal.

Let's wait to hear from @christianalfoni

@jjenzz
Copy link
Contributor

jjenzz commented Aug 19, 2020

I'm not sure about the compoundVariant suggestion tbh because it means styles are really spread out. I feel like it could get tricky to reason about what styles a particular variant would be in a given state.

What about something like this that keeps things together?

const Button = styled('button', {
	// ...
});

Button.variant('color', {
	red: {},
	blue: {},
});

// create an `appearance` variant that "depends" on `color`
Button.variant('appearance', {
	ghost: {
		background: 'transparent',
		border: '2px solid',
		// concatenates variant name and value from dependencies for property 
		// name to prevent conflicts with other dependencies and css properties
		colorRed: { borderColor: 'red' }, 
		colorBlue: { borderColor: 'blue' },
	},
}, ['color']); // dependencies (inspired by hooks)

The only problem with this is if you want to have styles for a blue, ghost that's small. The nesting would get intense 😬

Button.variant('appearance', {
	ghost: {
		background: 'transparent',
		border: '2px solid',
		colorRed: { borderColor: 'red' }, 
		colorBlue: { 
			borderColor: 'blue',
			sizeSmall: {},
		},
	},
}, ['color', 'size']);

That's one issue that wouldn't really occur with the compoundVariant approach:

Button.compoundVariant({
	appearance: 'ghost',
	color: 'red',
	size: 'small',
}, { 
	bg: 'red' 
})

hmmmm 🤔

@peduarte
Copy link
Contributor Author

peduarte commented Aug 19, 2020

@jjenzz Thanks Jenna <3 I would prefer to have them together too, something like what you suggested or the initial one. But it seems tricky or even impossible with typescript. I agree with the deep nesting you mentioned too, hmmm.

I think we could start with compoundVariant if everyone agrees, see how the users react to it. That'll give us time to work on a better API.

As long as it works, that's the main thing right now

@jjenzz
Copy link
Contributor

jjenzz commented Aug 19, 2020

The TS thing is a good point, especially since we can't do this microsoft/TypeScript#12754 and without concatenating, it would open a can of worms where color as a variant would conflict with color the CSS property.

Yeah, I'm starting to lean more towards compoundVariant now too tbh 👍

@hadihallak
Copy link
Member

NIce, so just to confirm, it seems that we all agree that the compoundVariant + the functional variant syntax is the preferable approach here?

@hadihallak
Copy link
Member

Chatted with Colm about this and we're going with this syntax for now while also making sure types will be working correctly:

Button.compoundVariant({
	appearance: 'ghost',
	color: 'red',
	size: 'small',
}, { 
	bg: 'red' 
})

@peduarte
Copy link
Contributor Author

Nice one!

@hadihallak
Copy link
Member

Resolved in #134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

No branches or pull requests

5 participants