[Icons] Move default icon colors to core

[llorca]

Any reason why this was in docs and not core?

[blueprint-bot]

Move default icon colors to core

Preview: documentation | table
Coverage: core | datetime

[cmslewis]

Yeah we intentionally did away with default icon colors in core, but we still wanted them to be a certain color when shown in the docs. See: #437.

Requesting changes because I think we shouldn't merge this.

[llorca]

Ha, I was looking for that issue earlier. Since then, there's also this one that happened: #1036

I wonder if that's good enough to bring back the default icon color into core. If not, I'll close this out!

A lot of folks forget to add those lines to their project:

.pt-icon,
.pt-icon-standard,
.pt-icon-large {
  color: $pt-icon-color;

  .pt-dark & {
    color: $pt-dark-icon-color;
  }
}

Is there anything we can do to simplify?

[cmslewis]

Hmm, based on my prior experience as a Blueprint consumer, I'd love to avoid a default icon color - gets annoying to override. I see icons as akin to letterforms, and our text doesn't ship with a default color, does it? Makes it that much easier to opt into styles via CSS classes (like pt-intent-* or pt-text-muted).

As for how to train folks to do this, should we just suggest that consumers use something more generic like $pt-{dark-}text-color-muted class whenever they're working with icons? Then we don't have to add any more styles to the library.

[llorca]

Looks like we do ship with a default text color (surprisingly not for .pt-dark... something wrong here): http://github.com/palantir/blueprint/blob/ac96ff2566e1225ad6f4bc952a755b0a3217778f/packages/core/src/_typography.scss#L27

[cmslewis]

Ah, interesting. Well I'm struggling to think of a good way to enforce reasonable icon colors without setting default colors explicitly. Maybe we should go through with this PR then? @giladgray @adidahiya thoughts here?

[sonovawolf]

If anything not having a default color for icons is detrimental to consistency. Just like text, we have very well defined colors people can use. Just like text, icons should render in their default color, and people can then change it explicitly through classes.

Even more important now that we are moving away from a world of custom apps.

[giladgray]

we removed the color property from .pt-icon-* in #437 because it caused more problems than it solved: users regularly had to write custom CSS with specific selectors to change icon colors. in PR #497 which fixed that issue, we made sure that all BP components that support icons properly set $pt-icon-color as expected.

A lot of folks forget to add those lines to their project

we never asked people to add those lines. nor should they add them, as that would bring them straight back to the pre-#437 world where they have to override icon color in any custom cases.

[giladgray]

instead of setting a default color property on icons (which has its own consistency problems described above), we provide the $pt-[dark-]icon-color Sass variable which folks can apply to their own icons that need the default coloring. it falls to product teams + designers themselves to enforce this.

[giladgray]

strongly against this PR, for reasons noted above.

[llorca]

@thisisalessandro That one will remain on you and other consumers for the time being. Not sure we can have a great solution for all the use cases.

[sonovawolf]

It falls to product teams + designers themselves to enforce this.

That's exactly the problem. Instead of enforcing the desired default behavior for the system (aka icons come in the default icon color) we are prioritizing the needs of people doing work for custom Palantir apps. At the end of the day those who will need to customize the icon color will have to write CSS anyway, but we could at least make it easier for those who don't.