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

Universal variations interface using "variant" prop #6130

Closed
kof opened this issue Feb 13, 2017 · 18 comments
Closed

Universal variations interface using "variant" prop #6130

kof opened this issue Feb 13, 2017 · 18 comments
Assignees
Milestone

Comments

@kof
Copy link
Contributor

kof commented Feb 13, 2017

We discussed this already in chat with @nathanmarks. I thought it would be good to have a public discussion on this. My idea is to have a component property variant which can be used to define a styling variation.

In case of a Button, raised or primary state can be implemented over a variant internally (and stay backwards compatible) as well.

This opens a door for different use cases, where you have a button but slightly changed, for e.g. with a different color or without a border or anything else. Using a theme user can define a custom variation and pass the variant over the props.

Example

const theme = createMuiTheme({
  Button: {
    variants: {
      special: {
        root: {
          borderRadius: 0
        }
      }
    }
  }
})

<ThemeProvider theme={theme}>
  <Button variant="special">Special Button</Button>
</themeProvider>
@kof kof changed the title Universal API using "variants" Universal variations API using "variant" prop Feb 13, 2017
@kof kof changed the title Universal variations API using "variant" prop Universal variations interface using "variant" prop Feb 13, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2017

Thanks for starting this conversion. That's an interesting topic!
Before we dive into it. Would you place the type property of the <Text /> under the same category?

We use that variation approach a lot at @doctolib. We expose a uiStyle property (named variation on this issue) on almost all our low-level components. I wish I could show you that, but it's not open sourced yet.
I definitely think that there is a need for it.

Also, I have seen a lot of people trying the customize the internal of our components to create a variation. For instance #6127 yesterday. As far as I have seen it. There is four different type of customization needs. Going from the less specific to the most specific:

  1. The Material Design variations like with the buttons.
  2. The User global theme variation.
  3. A specific variation of a component made by a user used in different context.
  4. A very specific variation for a one-time problem/situation.

Which need are we going to solve with the variation property? I'm really eager to see the API/implementation you are suggesting.

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

I think this will solve points 1-3, for just one elements its too much work to predefine a variation and wrap the component with a separate provider to pass that theme. Point 4 should be solved by a direct overwriting using a className prop (or/and classes), but lets not go into detail here lets focus on those first 3 things.

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

And by the way this can be mostly done in jss-theme-reactor, components only need to add one more class if a variant is defined.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2017

So, what about this API? We would cover all the customization needs I have seen so far. That's exciting ⭐️ !

import createMuiTheme from 'material-ui/styles/theme';
import createPalette from 'material-ui/styles/palette'

const themeRoot = createMuiTheme({
  /**
   * Use case 2.
   * User can change the global theme,
   * impacting all the instances and all the components.
   */
  palette: createPalette({
    primary: brown,
    accent: deepOrange,
    type: 'light',
  }),
  components: {
    Button: {
      /**
       * Use case 2.
       * Users can change all the instances of the Button component.
       * `default` is the default variation used.
       */
      default: {
        root: {
          borderRadius: 0,
        },
      },
      /**
       * Use case 3.
       * Users can have a variation of a component.
       * Applying the same on different contexts.
       */
      special1: {
        root: {
          borderRadius: 5,
        },
      },
    },
  },
})

/**
 * Use case 3. / 4.
 * By allowing the nesting of `<ThemeProvider />` users
 * can customize things even further.
 */
const themeNested = createMuiTheme({
  components
    Button: {
      special2: {
        root: {
          borderRadius: 15,
        },
      },
    },
  },
})

<ThemeProvider theme={themeRoot}>
  {/* borderRadius: 0 */}
  <Button>Special Button</Button>

  {/* borderRadius: 5 */}
  <Button variant="special1">Special Button</Button>

  <ThemeProvider theme={themeNested}>
    {/* borderRadius: 15 */}
    <Button variant="special2">Special Button</Button>
  </ThemeProvider>

  {/* Use case 4. */}
  <Button style={{borderRadius: 20}}>Special Button</Button>
</ThemeProvider>

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

Oh wait, we have already this:

const theme = {
  palette,
  overrides: {
    LinearProgress: {...},
    MenuItem: {...}
  }
}

So maybe it makes sense to just add the variants on the same level as overrides? It is a bit of a duplication for the component name, but less nesting instead.

const theme = {
  palette,
  overrides: {
    Button: {...}
  },
  variants: {
    special: {
      Button: {...}
    }
  }
}

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

You have still an error:

      special1: {
        borderRadius: 5,
      },

special1 is a variant name, its value is a styles object, like this:

special1: {
  root: {
    borderRadius: 5
  }
}

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

One more thing to consider is that all rule names you can overwrite using variants and overrides are considered part of the public api, because a user who creates a theme using variants and overrides will rely on those names, once you change them, you break users theme. Those names should be well chosen and their changes considered public api change with major version increment.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2017

I have made another iteration.

considered part of the public api

That would be my only concern: increasing the API surface so users don't have to create wrapping components
That's definitely another solution: advising users to use the className API to wrap component when they need to.
But are we really going to increase the API surface? From my experience with the master branch. Users want to be able to override everything. Over time, we added properties to target elements and different states. Exposing the rules will just make the process simpler and more performant.
Still, users have to understand that overrides at the rules level is fragile and they might suffer breaking changes.

@kof
Copy link
Contributor Author

kof commented Feb 13, 2017

Well, basically we already exposed the rule names as a public api by adding "overrides". From the moment it is documented rule names are not any more an internal implementation detail.

users have to understand that overrides at the rules level is fragile

I think if its documented then its a public and shouldn't be fragile. We still can change names, but we need to increment major version to follow semver.

@acroyear
Copy link
Contributor

acroyear commented Feb 13, 2017

This still seems to require defining the variations in the theme as it is declared and initialized. It doesn't suit my use case which is to have the components change their color based on some arbitrary artwork on the screen (e.g., a photo or an album cover). I can do that in FAB, and a few other components, but not in slider without creating a new theme on demand (per #6127 comment).

(ok, it kinda does given the nesting part...)

@kof
Copy link
Contributor Author

kof commented Feb 14, 2017

@acroyear
For this you will want to pass it over props, its the 4. point in @oliviertassinari list and we said this issue is about first 3.

Also you will be able to do that in your user component using dynamic values cssinjs/jss#356 wich are comming to jss.

But still doing animation is something you can do more efficiently with inline styles or with direct sheets manipulation

@kof
Copy link
Contributor Author

kof commented Feb 14, 2017

Created this issue #6137 issue to discuss the 4th case.

@kof
Copy link
Contributor Author

kof commented Feb 14, 2017

Btn @oliviertassinari we can make styles overrides less fragile if we flowtype all styles.

@kof
Copy link
Contributor Author

kof commented Feb 14, 2017

In fact theming using overrides is fragile in all frameworks on the web, because nobody specifies every CSS Rule property as part of the public API, so every time you override styles using class names, you expect certain styles defined by the framework, but there is no guarantee those props will be defined in any future version because they have been never part of the public API.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 14, 2017

I think if its documented then its a public and shouldn't be fragile. We still can change names, but we need to increment major version to follow semver.

Sure, they are breaking changes. What I mean is that people might experience many breaking changes as we refactor the implementation. It's a power they can use, but we should warn them about the consequences.

we can make styles overrides less fragile if we flowtype all styles.

I don't have much experience with flow, but I guess you are right. That's something to look at.

@kof
Copy link
Contributor Author

kof commented Feb 22, 2017

Right now struggling how to implement a "danger" button its a red button which will remove stuff. I couldn't find any predefined variants for this. Could be easily implemented using suggested variants api.

@jcheroske
Copy link

jcheroske commented Jul 3, 2017

Just curious what's happening with this? Has a final API been decided? This type of thing seems like a key piece of the puzzle, as it addresses the need for dynamic styling in situations not best handled by function values.

I recently ran into needing this, and rolled my own HOC that allows for any number of "variant" type props to be declared on a component. Here's an example that uses a theme to prop mapping function. It declares a prop called validity that only accepts the allowedVariants. The HOC also supports a mapThemeToStyles function as well, for mapping from the theme to one or more JSS styles.

const variantConfigs = [
  {
    allowedVariants: ['error', 'valid', 'unvalidated'],
    defaultVariant: 'unvalidated',
    mapVariantToProps: v => ({
      errorStyle: {
        color: v.errorColor
      },
      labelStyle: {
        color: v.color
      },
      underlineStyle: {
        borderColor: v.underlineColor
      }
    }),
    propName: 'validity'
  }
]

const enhancer = configureVariants({
  themePath: 'components.selectField.variants',
  variantConfigs
})

@oliviertassinari
Copy link
Member

I don't think that we should move that logic in the core of the styling solution given it can be implemented in userland. Personally, I have been implementing by hand (I mean without abstraction), that's fine. I also like the solution proposed by @jcheroske for industrializing it.

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

No branches or pull requests

5 participants