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

[docs] Document and exposes colorManipulator helpers #10789

Closed
1 task done
rocketraman opened this issue Mar 24, 2018 · 9 comments
Closed
1 task done

[docs] Document and exposes colorManipulator helpers #10789

rocketraman opened this issue Mar 24, 2018 · 9 comments
Labels
duplicate This issue or pull request already exists

Comments

@rocketraman
Copy link
Contributor

rocketraman commented Mar 24, 2018

  • I have searched the issues of this repository and believe that this is not a duplicate.

In recent beta's it looks like a lot of nice simplifications have been made to the theme palette approach. However, there is one thing which is unclear to me. In my old code, I was able to have logic like this:

theme.palette.primary[200]

The closest thing I seem to have now is to use colorManipulator.lighten and darken, passing in a relative value as the "coefficient" e.g.

lighten(theme.palette.primary.main, 0.25)

Is this the "right" approach to use now? If so, the docs should probably describe the best way to use these methods, and typical values of the coefficient value to achieve various shades in the source palette. For example, is it true to say that, assuming primary.main is the equivalent of primary[500] before, then primary[200] would now be achieved via a coefficient of 0.4, since 200/500=0.4?

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Mar 24, 2018
@oliviertassinari oliviertassinari changed the title [DOCS] Document new palette equivalent of theme.palette.primary[n] [docs] Document colorManipulator helpers Mar 24, 2018
@oliviertassinari oliviertassinari changed the title [docs] Document colorManipulator helpers [docs] Document and exposes colorManipulator helpers Mar 24, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 24, 2018

@rocketraman There are no official alternatives to the theme.palette.primary[200] style. You can always use the color[200] style when using an official Material Design color.

Right now, the colorManipulator helper functions are considered a private API, they aren't exposed in the index nor documented. Should we make it a public API? I don't know. Color manipulation isn't our core mission, maybe there is better open source solution out there. But at the same time, ours are stable and haven't changed in a long time. So why not.

You can find some shared pain points with #10499.

@mbrookes
Copy link
Member

mbrookes commented Mar 24, 2018

@rocketraman The palette is primarily about giving control over the colors that Material-UI components use, but don't forget that you can also add custom keys to the theme, so you can easily provide additional colors to use in your own components:

import { createMuiTheme } from 'material-ui/styles';
import red from 'material-ui/colors/red';
import green from 'material-ui/colors/green';
import blue from 'material-ui/colors/blue';

const theme = createMuiTheme({
  palette: {
    red,
    green,
    blue,
  },
});

Or, as @oliviertassinari mentioned, just import them directly in your component.

For example, is it true to say that, assuming primary.main is the equivalent of primary[500] before, then primary[200] would now be achieved via a coefficient of 0.4

No, because the Material Design colors don't follow any standard gradient, and hue and saturation are also tweaked variously across colors. There is no way to apply a standard function to a 500 color to arrive at some other color matching the spec.

@rocketraman
Copy link
Contributor Author

rocketraman commented Mar 24, 2018

@oliviertassinari @mbrookes Both of those are useful comments, thanks. If some of the functions in colorManipulator were exposed and documented, the two concepts could be combined nicely when building up a theme. For example:

import { createMuiTheme } from 'material-ui/styles';

const primaryMain = '#0ccaff';
const theme = createMuiTheme({
  palette: {
    primary: {
       main: primaryMain
    },
    // additional theme colors
    lightlyAccentedSelection: lighter(primaryMain, 0.2),
  },
});

@mbrookes
Copy link
Member

If you only add primary.main as here, then createPalette() (as part of createMuiTheme) will add the missing light and dark keys, using, funnily enough, 0.2 as the coefficient for lighten, but I take your point - it might be some other value for your custom key.

However unless primaryMain is a variable, for example a user defined color scheme, lightlyAccentedSelection might as well be a fixed color appropriate to your color scheme.

That said, I have no objection to colorManipulator being documented. There are much more comprehensive solutions available, but it could save a few bytes in your build if the available functions were sufficient.

@rocketraman
Copy link
Contributor Author

rocketraman commented Mar 24, 2018

it might be some other value for your custom key.

Yes, this was just an example, of course. Good to know 0.2 is the default for light though.

However unless primaryMain is a variable, for example a user defined color scheme

Yes, that is the case for me. The docs point people to the material IO color tool, and often times people will use the Custom functionality to choose another main primary color. In my case, I've chosen a custom primary main based on my "brand" identity, so I am unable to use any of the pre-defined colors.

That said, I have no objection to colorManipulator being documented. There are much more comprehensive solutions available, but it could save a few bytes in your build if the available functions were sufficient.

Are you thinking about something like https://github.com/bgrins/TinyColor?

I'm wondering if colorManipulator then should expose publicly a material-specific API? For example, given some input color x, allow me to get x[n] for it, where n is one of the standard material spectrum values.

@mbrookes
Copy link
Member

@rocketraman I was referring to your users, and giving them the ability to change the color of your app, bet even then you'd probably want to provide a predefined set of options.

Since your primaryMain is a const, then there's no reason your lightlyAccentedSelection can't be a static color, rather than calculating it on the fly.

given some input color x, allow me to get x[n]

I don't know that that's possible (or at least that simple) given that, as mentioned, for any particular hue, the shades don't follow a standard pattern. Try working out how much to lighten red[500] by to get say, red[200], then apply the same to other hues and see how well they match up with the spec.

@rocketraman
Copy link
Contributor Author

Since your primaryMain is a const, then there's no reason your lightlyAccentedSelection can't be a static color, rather than calculating it on the fly.

Yeah, that makes sense. Well, no reason other than the self-documenting program that results from a calculated value rather than a static value. Same reason people define constants like:

const followupInSeconds = 10 * 60 * 60

But yeah, I can always add a comment instead...

I don't know that that's possible (or at least that simple) given that, as mentioned, for any particular hue, the shades don't follow a standard pattern. Try working out how much to lighten red[500] by to get say, red[200], then apply the same to other hues and see how well they match up with the spec.

Fair enough. I didn't realize the predefined color shades were customized by Google to such an extent. The existing lighter and darker methods, with some documentation on the general meaning of the coefficient would be sufficient then, I think.

@oliviertassinari oliviertassinari added new feature New feature or request and removed discussion labels Sep 29, 2018
@oliviertassinari oliviertassinari added component: time picker This is the name of the generic UI component, not the React module! priority: important This change can make a difference duplicate This issue or pull request already exists and removed component: time picker This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation new feature New feature or request priority: important This change can make a difference labels Mar 13, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 13, 2019

I'm closing for #13039.

@mbrookes

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

3 participants