search

vaadin / react-components

13 stars 4 forks source link

Button variant should be property #254

Open abdullahtellioglu opened 2 months ago

abdullahtellioglu commented 2 months ago

It is not intuitive to set the primary variant via theme property.

Non-intuitive: 

<Button theme={"primary"} onClick={() => setEveryone(false)}>

Intuitive: 

<Button primary onClick={() => setEveryone(false)}>
rolfsmeds commented 2 months ago

I don't think we should "pollute" the property space of our components by adding a dedicated property to every single theme variant of every component. Would be nice if we could provide the available option via TypeScript, but as they're not (in all cases) mutually exclusive, I'm not sure how that would be done.

Other component libraries also have similar string properties for their variants, e.g. MUI has a type property on their Button that works similarly to our theme.

abdullahtellioglu commented 2 months ago

You are right, It does not matter how you define it -type="primary"`, `primary`, `theme="primary"- as long as TS shows available options and warnings/errors for invalid values.

web-padawan commented 2 months ago

The main problem is that we have two themes (Lumo and Material) in the web components and they have different values:

Furthermore, our TypeScript classes are placed in src folders and they don't know anything about theme values.

Another problem is that theme prop is only declared in React wrappers. In web components the theme is supposed to be set using attribute, not a property (the attribute has matching _theme property but it's intentionally protected).