Palet colors required with get-color

[samsch]

The docs suggest that you can remove any $foundation-palette color except for primary. However, _global.scss uses get-color for all 5 defaults without checking that they exist (except a check for primary). So foundation fails to compile when $foundation-palette has colors removed, since get-color throws if a color doesn't exist.

I did not specifically test each color, but it definitely failed with no 'alert' color. It looks like it should fail for each color on lines 107-110 of _global.scss.

In 6.2, map_get would return null, but not error for missing map keys. This allowed the compilation to continue, with the default color vars set to null if their color didn't exist.

How to reproduce this bug:

Setup compiling Foundation-Sites (or parts) with sass.
Configure $foundation-palette to not include alert or other default colors (except primary, which is documented as required).
Attempt to compile.

What should happen:

It should compile fine.

What happened instead:

Compile error like @error 'given $key is not available in $foundation-palette';.

Resolution

The easy solution would be to change the docs to state that the default colors are required. This doesn't seem like a great solution to me, but it is valid.

A better solution would be to remove the dependence on the specific colors. Possibly changing get-color to take an option argument which makes it return null rather than error for a missing color. It looks like alert is additionally expected to exist in forms/_error.scss and settings/_settings.scss.

[IamManchanda]

cc / @DaSchTour @natewiebe13

[DaSchTour]

I guess the old variables that create the error could be removed or need's protection by a map has key guard. This variables were used before $foundation-palette was introduced. So best would be to remove them as they are obsolete and should not be used internally anymore.

[kball]

cc @andycochran

[andycochran]

When I A11y'd the color palette, I added this warning statement to the docs, knowing that removing default colors could cause errors:

If you remove a default key from $foundation-palette, be sure to edit any variables in your settings file that reference that color.

That was for the sake of components erroring, but it doesn't account for those internal global vars. However, those vars are unnecessary if you also remove them from your settings file (as the warning statement advises). So maybe the fix is to make the internal vars conditional.

But I think @DaSchTour is right; it'd be great if we could completely remove them.

I'll look into a fix… 

[andycochran]

@DaSchTour we could fix this by changing the internal ("internal" is inaccurate) color vars in _global.scss to something like:

// Internal variables used for colors
@if map-has-key($foundation-palette, primary) {
  $primary-color: map-get($foundation-palette, primary);
} @else {
  $primary-color: #1779ba;
}
@if map-has-key($foundation-palette, secondary) {
  $secondary-color: map-get($foundation-palette, secondary);
} @else {
  $secondary-color: #767676;
}
@if map-has-key($foundation-palette, success) {
  $success-color: map-get($foundation-palette, success);
} @else {
  $success-color: #3adb76;
}
@if map-has-key($foundation-palette, warning) {
  $warning-color: map-get($foundation-palette, warning);
} @else {
  $warning-color: #ffae00;
}
@if map-has-key($foundation-palette, alert) {
  $alert-color: map-get($foundation-palette, alert);
} @else {
  $alert-color: #cc4b37;
}

[DaSchTour]

@andycochran well there is already a add-foundation-colors() function to add the deprecated variables with values from $foundation-palette.
With #9683 there also will be major changes to color handling. So I'm not sure if there is any need to change something.
I think the problem here is, that removing what is called state-colors in #9683 may break generation of buttons, callouts and so on. So the issue mentioned here will be avoided as all components will depend on $foundation-states.

[kball]

Basic version of this fixed in #10479. Going to close this, as I think this handles hte intent of this issue... going to bump #9683 though as I'd like to see that conversation revitalized.