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

Primary color does not match the theme #9857

Closed
1 task done
ralphsmith80 opened this issue Jan 13, 2018 · 19 comments
Closed
1 task done

Primary color does not match the theme #9857

ralphsmith80 opened this issue Jan 13, 2018 · 19 comments
Assignees
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation

Comments

@ralphsmith80
Copy link

Looking at the default theme I would Based on the demos I expect the button background color to be #2196f3 but it's actually #3f51b5. Possibly the demo page is just using a different demo theme and I'm not understanding how to extend the existing light theme.

Ultimately I want to be able to override the default theme to provide a new primary and accent color. I in the past I've used the pre 1.0 steps. Now it is unclear to me from the docks how to do this without providing an entire new theme. That's a lot when I just want to override a few variables.

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

Expected Behavior

Primary theme color should be used

Current Behavior

After files are processed through webpack from create-react-app setup (not ejected) the background color for appbar and button is #3f51b5

Steps to Reproduce (for bugs)

https://codesandbox.io/s/612mpxw7kn

  1. Add a button as follows
<Button raised color="primary">
  Default
</Button>
``

## Context
Ultimately I want to be able to override the default theme to provide a new primary and accent color. I in the past I've used the [pre 1.0 steps](http://www.material-ui.com/#/customization/themes). Now it is unclear to me from the docks how to do this without providing an entire new theme. That a lot when I just want to override a few variables.

## Your Environment
App was created from `create-react-app 1.4.3`

```package.json
{
  "dependencies": {
    "material-ui": "^1.0.0-beta.27",
    "material-ui-icons": "^1.0.0-beta.17",
    "react": "^16.2.0",
    "react-dom": "^16.2.0",
    "react-redux": "^5.0.6",
    "react-router-dom": "^4.2.2",
    "react-scripts": "1.0.17",
    "redux": "^3.7.2",
    "typeface-roboto": "0.0.50"
  },
  "lint-staged": {
    "src/**/*.{js,jsx,json,css}": [
      "prettier --single-quote --no-semi --trailing-comma all --jsx-bracket-same-line --write",
      "git add"
    ]
  },
  "scripts": {
    "precommit": "lint-staged",
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  },
  "devDependencies": {
    "husky": "^0.14.3",
    "lint-staged": "^6.0.0",
    "prettier": "^1.9.2"
  }
}
@oliviertassinari oliviertassinari added the support: question Community support but can be turned into an improvement label Jan 13, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 13, 2018

@ralphsmith80 You can learn more about theme palette configuration in the documentation.
Notice that we plan on revamping the API in #9794.

@mbrookes mbrookes added bug 🐛 Something doesn't work docs Improvements or additions to the documentation and removed support: question Community support but can be turned into an improvement labels Jan 13, 2018
@mbrookes
Copy link
Member

@oliviertassinari @ralphsmith80 is correct, we're displaying the current theme, rather than the default theme, so the primary key is listing blue values (the docs primary color) rather indigo (the default).

@mbrookes mbrookes reopened this Jan 13, 2018
@oliviertassinari
Copy link
Member

@mbrookes Oh right 👍

@oliviertassinari
Copy link
Member

@ralphsmith80 Thanks for raising the issue.

@ralphsmith80
Copy link
Author

Thanks guys! You pointed out a major missing expectation. The demo page does not represent the default theme. Not sure if there are plans to align that when 1.0 is out of beta, but I think it would be a good idea so the demo represents what you get out of the box.

Also another point of confusion is around the palette example. Overriding A400 has no effect in the example as the accent color being used is A200. While I understand accent colors are prefixed with A it's not clear which variant (100-700) is used for what state (active, hover, focused, etc...).

I don't know if it's worth filing a different issue for that or not.

@mbrookes
Copy link
Member

I don't know if it's worth filing a different issue for that

Nope, as @oliviertassinari mentioned: #9857

@oliviertassinari
Copy link
Member

@mbrookes What do you think of making the current documentation palette the default palette? It sounds simpler.

@mbrookes
Copy link
Member

mbrookes commented Jan 14, 2018

So it seems there are a few options:

  1. Change the docs to match the "default" color as @ralphsmith80 suggests. (Boring! 😜 )
  2. Change the dafault color to match the docs. (Material Design doesn't have a default per-se. I believe ours was chosen to match the examples given here: https://material.io/guidelines/style/color.html#color-color-system - scroll to Primary color / Secondary color, but nothing says they have to be.)
  3. Wrap the DemoComponent in the default theme, so the demos match the default indigo, while the overall docs retain the blue theme.
  4. Mix it up and give the demos a selection of colors, just as the MD spec examples do so there is no expectation.
  5. Maintain the status quo. (@ralphsmith80 described this as a "major" expectation, but so far, no-one else has expressed any confusion.)

It isn't a priority in any case.

@oliviertassinari
Copy link
Member

@mbrookes Option 5 sounds good to me.

@finnhvman
Copy link

I would like to put my two cents in.

It's not clear to me whether the light and dark keywords are for normal or accent colors. However, I noticed that the dark color is used in the light theme as the ink color on the Textfield and Input components for example. According to the Material Guidelines:

Color: 100% primary color (Dark A700)

should be used as ink in the light theme, which is the accent color. Therefore I assume that the light and dark keywords represent accent colors, if I want to comply the guidelines.

@yihangho
Copy link
Contributor

I agree that option 5 is good enough. However, I think it will also be very helpful if the documentation pointed out that it is not using the default palette, hence differences should be expected. The reason is that it is possible that someone (like myself) was wondering if it is caused by some kind of misconfigurations (e.g., missing stylesheets, etc).

@mbrookes
Copy link
Member

mbrookes commented Mar 15, 2018

@yihangho Where in the docs would you expect to find that? (With such extensive documentation, discoverability can be hard for new users.)

https://material-ui.com/style/color/
https://material-ui.com/customization/themes/
https://material-ui.com/customization/default-theme/
https://material-ui.com/getting-started/faq/

Somewhere else?
Some / all of the above? :)

@mui mui deleted a comment Mar 15, 2018
@yihangho
Copy link
Contributor

yihangho commented Mar 16, 2018

@mbrookes Among the links you posted, I think the getting started FAQ. However, for some reason, I didn't refer to that.

Initially, I checked the installation guide because I remember there are references to external deps (the fonts), so I thought it might be documented there. After that, I went to https://material-ui.com/customization/default-theme/ to diff the color code against what I'm seeing.

All in all, I think a brief note at the following pages will help a lot:

https://material-ui.com/getting-started/faq/
https://material-ui.com/customization/default-theme/

@mbrookes
Copy link
Member

@yihangho Great! Would you like to add it?

@yihangho
Copy link
Contributor

@mbrookes Will do!

@alexboots
Copy link

alexboots commented Sep 24, 2018

(@ralphsmith80 described this as a "major" expectation, but so far, no-one else has expressed any confusion.)

Just want to chime in and say I got super confused by this as well.

Tore my app apart to find my theme mistake and it took a while to realize it out-of-the-box defaults to primary: indigo / secondary: pink.

Obviously I didn't think to look at the FAQ to see if there was a reason for this (thanks for adding that @yihangho !)

Part of my confusion came from setting color="default" on the from the component demo page sets the background to white, vs leave that off sets the background is indigo - so copy / pasting the first two examples displays two different themes locally.

I'd vote on any of 1, 2, 3 or 4 as a way to help lessen this confusion.

I'm happy to make one those changes in to docs if others are confused and run into this thread.

@mbrookes
Copy link
Member

mbrookes commented Sep 24, 2018

@alexboots

1 is out - we've settled on that color for the logo.
2 would be a breaking change - users apps would change color
3 is going to look odd if the AppBar doesn't match the demos
4. is the most work, and might suffer from the same problem as 3 unless the colors are chosen carefully.

I can't pick a better winner than 5, other than adding the same note in other parts of the docs. (Default theme page seems logical...)

@ralphsmith80
Copy link
Author

@mbrookes I think you caught the quote with my tag instead of @alexboots comment.

Obviously this isn't a problem for me now that I know about it. If people are having problems with this then based on the fact that material-ui is reving so quickly I think you could make a breaking change and just put in a breaking change release.

It sound like changing the logo color is not possible - maybe for brand reasons so I personally would go with (2). I like the demo page colors better anyway but that's just my opinion.

@mbrookes
Copy link
Member

@ralphsmith80 Oops, apologies, yes, corrected.

After the 3.0 shenanigans, the next planned breaking release isn't expected for 6 months. However given that @alexboots is the first person in 6 months to tag this issue, I'm reluctant to change anything. If someone wants to enhance the documentation in the meantime, I'm good with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation
Projects
None yet
Development

No branches or pull requests

6 participants