bug: Incorrect toolbar-title colour

[ctcampbell]

Type: bug

Ionic Version: 2.x

Platform: all

I have set $colours.primary to #FD6C96 and this results in the toolbar-title text being set to black when white would be more appropriate. I also get this for #90EE90.

[adamdbradley]

I agree the #FD6C96 should be white text, but #90EE90 being white seems a little too hard to read:

What you can do is override the inverse sass function: https://github.com/driftyco/ionic/blob/2.0/ionic/util/functions.scss#L5

[ctcampbell]

I guess it depends on the device as white looks fine on a nexus 5. I would say that the only time black is appropriate is when the background is white or on some shades of grey. Black on a colour is never going to look good. The text colour is probably better off being a developer controlled value rather than trying to create it dynamically.

[DILEEP-YADAV]

@ctcampbell that's right. color is identity of branding. it does't matter any OS , App should be free for own look and feel, But that's material design good for android.

[adamdbradley]

@ctcampbell @DILEEP-YADAV Yeah I agree that it needs to be easier to developers to get the exact colors they want. The challenge we have is that we also make it easy to set all the colors using a sass color map, but not an overly complicated color map. With the map you can decide how many colors you want and what color names you want. However, like you've both pointed out, it also needs to sometimes figure out if the text would be better black or white on top of the base color, which is what the inverse sass function does. Right now this is the default color map:

$colors: (
  primary:    #327eff,
  secondary:  #32db64,
  danger:     #f53d3d,
  light:      #f4f4f4,
  dark:       #222,
)

A goal of mine is to keep that map a simple as possible, so you can quickly understand what it's doing and how to update it. To add the inverse color to the map, it'd end up being like:

$colors: (
  primary:    (
      base: #327eff,
      inverse: white
  ),
  secondary:    (
      base: #32db64,
      inverse: white
  ),
  danger:    (
      base: #f53d3d,
      inverse: white
  ),
  light:    (
      base: #f4f4f4,
      inverse: black
  ),
  dark:    (
      base: #222,
      inverse: white
  ),
)

It's not horrible, but I think it adds a complexity to an already complex concept for CSS: dynamically creating any number of colors with any name of your choosing.

So what if we did the best of both worlds, where the first example is the standard way, but if there is a case where the auto inverse color isn't to the developers choosing, they can also write it the second way. So something like below could be possible.

$colors: (
  primary:    (
      base: #327eff,
      inverse: black
  ),
  secondary:  #32db64,
  danger:     #f53d3d,
  light:      #f4f4f4,
  dark:       #222,
)

So the question is, how can we easily allow developers to choose color names and values, the number of colors they want, and each color's inverse color, but also, how can we keep it simple and easy to understand? Thoughts? @brandyscarney

[adamdbradley]

Also, I did update the inverse function to use white in more scenarios, which should fix your immediate problem. 55ef5a8

However, the issue still exists for when developers what to be specific on which foreground color to use.

[DILEEP-YADAV]

OK ! Great work

[brandyscarney]

@adamdbradley I like the idea of having it use our default colors unless otherwise specified by the developer, but I think this also goes back to the need for more customization of the tabs. Material Design has the tab text color, tab highlight color, and the background color that can all be customized.

So we could go the simple, less to write route of just passing multiple colors, but do you think it would be confusing? Like primary below.

$colors: (
  primary:    (#327eff, white),
  secondary:  (#32db64),
);

@each $color-name, $color-values in $colors {
    $bg-color: nth($color-values, 1);
    $fg-color: inverse($bg-color);

    @if (nth($color-values, 2)) {
      $fg-color: nth($color-values, 2);
    }
}

Or we could do like you said and be really specific with what each one does, but I don't know if base and inverse are the right words. This would allow us to treat each color like a configuration, people could pass the tab highlight color they want and get super custom with it.

$colors: (
  primary: ( bg-color: #327eff, fg-color: white ),
  secondary: ( bg-color: #32db64, fg-color: black, tab-highlight: yellow ),
);

$color: map-get($colors, $color);
$bg-color: map-get($color, bg-color);
@if map-has-key($color, fg-color) {
  $fg-color: map-get($color, fg-color);
}
@if map-has-key($color, tab-highlight) {
  $tab-highlight: map-get($color, tab-highlight);
}

Disclaimer: this code may not be entirely functional, not tested

[ctcampbell]

As a user, I like this option

`$colors: (
primary: (
base: #327eff,
inverse: black
),
secondary: #32db64,
danger: #f53d3d,
light: #f4f4f4,
dark: #222,
)``

though one thing I would say is it probably makes sense to use a naming convention that is similar (if not identical) to the material design naming convention. I agree 'inverse' isn't especially informative.

[brandyscarney]

I'm not sure if we want to go entirely material design since these are also used for iOS. However, thinking about it more I realized that the color isn't always used as a background color on some components (e.g a clear button) so maybe we could just get extra specific with what they do. Still thinking.

[brandyscarney]

We decided to use the word contrast since this color is sometimes used as a text-color but also used in other places. To us this made the most sense. With the release of beta.4 you'll be able to do something like this (for example) in your app.variables.scss file:

$colors: (
  primary: (
    base: #327eff,
    contrast: yellow
  ),
  secondary: (
    base: #32db64,
    contrast: hotpink
  ),
  danger: #d91e18,
  light: #f4f4f4,
  dark: #222
);

which will result in the following look for some components:

and you can easily change this to:

$colors: (
  primary: (
    base: #54abee,
    contrast: #551a8b
  ),
  secondary: #32db64,
  danger: (
   base: #d91e18,
   contrast: #18d3d9
  ),
  light: #f4f4f4,
  dark: #222
);

and get this:

Notes:

• If you use base you must have a contrast and vice versa at the moment.
• The tab highlight for MD mode will match the contrast.
• If you do not provide a contrast it will use our default function to determine the color.

If you'd like to test this out sooner than the beta.4 release, check out the steps here to develop against the ionic 2.0 branch: https://github.com/driftyco/ionic/tree/2.0/scripts#developing-against-ionic-locally

Please let me know if you have any questions or complaints! 😄

[tleguijt]

Great new feature!
But it seems my sass compiler has some trouble with this format;

Error: (base: #02c2c7, contrast: #fff) isn't a valid CSS value.
        on line 5 of src/scss/variables/ionic.scss
>>     base: $color-turquoise,
   ----^

My definition:

$colors: (
  primary: (
    base: $color-turquoise,
    contrast: #fff
  ),
  secondary: (
    base: $color-turquoise-darker,
    contrast: #fff
  ),
  danger:     #460D0D,
  light:      #f4f4f4,
  dark:       $color-turquoise-darkest
);

I'm using Sass 3.4.22

[brandyscarney]

Hey @tleguijt! Could you create a separate issue for this so anyone else that runs into it will be able to find it. Also, are you using map-get anywhere in your app?

[nfleet1080]

@tleguijt @brandyscarney I also get this same error during build, however it does not seem to prevent the SASS from compiling. I also want to point out that color: color($colors, primary); and color: color($colors, primary, contrast); seem to be correctly pulling the base and contrast color values set in app.variables.scss despite the error.

[brandyscarney]

@nfleet1080 Thanks for the info! I was able to reproduce it with a call to map-get from another file. Like this:

app.variables.scss:

$color-turquoise: #00e5ee;

$colors: (
  primary: (
    base: $color-turquoise,
    contrast: #fff
  ),
  secondary:  #32db64,
  danger:     #f53d3d,
  light:      #f4f4f4,
  dark:       #222
);

schedule.scss:

$categories: (
 ionic: map-get($colors, primary),
 angular: #AC282B
);

The problem is map-get will only get one layer of the map, so to get the base or contrast using map-get it has to be nested like:

color: map-get(map-get($colors, primary), base);

the color function was created to avoid having to write it this way.

Are you using map-get anywhere?

[brandyscarney]

I found an issue also with using the color function in any of the app theming files because the function wasn't available yet. As a solution, you should import globals.core at the beginning of your app.variables.scss file:

// http://ionicframework.com/docs/v2/theming/

// Ionic Shared Functions
// --------------------------------------------------
// Makes Ionic Sass functions available to your App

@import 'globals.core';

I'll be adding this to the changelog and starters. I updated the conference app so that it will work if you add a base/contrast:

ionic-team/ionic-conference-app@66821af

[nfleet1080]

@brandyscarney I was originally using map-get before the update to beta 4, it was actually not until I read the changelog that I learned the color function was a thing that exists, as I don't recall ever reading about it in the theming section of the v2 docs site. I replaced all of my map-get calls to use the color function and when I recompiled I did not see the error again. Cheers!

Good to know about importing globals.core to use the function in the app theming files, I'll definitely make that adjustment as well. Thanks!

[brandyscarney]

@nfleet1080 You're right, the theming section needs some love. I created an issue for this on the site repo: ionic-team/ionic-site#534

Glad everything is working for you now! Thanks for working through this with me. 😄

[tleguijt]

@brandyscarney yes, thanks for pointing that out, I still had one map-get which I used for the primary color, on which it failed (altho the sass output wasn't all too clear about that; it seemed like the error occurred in the color definition itself, not in the definition where the color map is used)

[DILEEP-YADAV]

Awesome team work, Thanks ionic team and GitHub platform! ionic v2 ready to boom! still angular v2 is not ready to production . But I think ionic v2 is ready. thanks once again to great ionic team.

[raydelto]

I'm still struggling to change the text color of the navigation bar. Any definitive solution for the latest IONIC 2 version (2.0.0-beta.10)?

[MichelleDiamond]

It appears that IONIC 2 Sass is very buggy.. Original sass theme files are using eg : base: #123456 which conflicts with contrast: #123, or so terminal screams..

[brandyscarney]

@MichelleDiamond I'm not sure I fully understand the problem you are running into. I tried the following in the conference app and don't see any issues:

$colors: (
  primary:   (
    base: #123456,
    contrast: #123
  ),
  secondary:  #32db64,
  danger:     #f53d3d,
  light:      #f4f4f4,
  dark:       #222
);

Please visit our forum or slack channel if you have questions about the framework. 😄

If you believe this is an issue with the framework, please create a new issue with answers to the pre-populated questions.

[brandyscarney]

I am going to lock this issue since the original issue has been resolved. We have theming documentation on this here: http://ionicframework.com/docs/theming/

There are some detailed docs on the base and contrast here: http://ionicframework.com/docs/theming/theming-your-app/

If you are getting errors with the sass, we ask that you please create a new issue by filling out the provided template. The more information we have the easier it is to help. 😄

If you have questions on theming (or anything else in the framework) please visit our forum or slack channel.

If you think we need some better documentation, please create a new issue on our site repo: https://github.com/ionic-team/ionic-site

Thanks everyone!