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

Fixed .callout text- and link-colors #9098

Closed
wants to merge 2 commits into from

Conversation

wolfgang-demeter
Copy link
Contributor

  • .callout text color now uses foreground() with previously unused $callout-font-color-alt and is now aware of its background-color.
  • .callout links now consider $callout-link-tint and are also aware of its background-color. $callout-link-tint is mentioned in Sass Reference and in _settings.scss but wasn't used at all!

- .callout text color now uses foreground() with previously unused
$callout-font-color-alt and is now aware of its background-color.
- .callout links now consider $callout-link-tint and are also aware of
its background-color. $callout-link-tint is mentioned in Sass Reference
and in _settings.scss but wasn't used at all!
@wolfgang-demeter
Copy link
Contributor Author

closing and reopening to skip travis build failure (Failed at the phantomjs@1.9.7-15 install script 'node install.js'.)

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @wolfgang-demeter, thank you for your PR !

That's great you fixed an unused parameter. But it put the spotlight on this parameter and its usefulness :

  • I don't understand why we should change the lightness of a link inside a callout.
  • By selecting links with a and not .callout-link, all links are changed, even inside other components inside the callout (high specificity).
  • a:not(.button) does not ensure that button (or similar components) links are not changed. Buttons can be declared in an other class.

poke @kball

@kball
Copy link
Contributor

kball commented Oct 10, 2016

I concur on using .callout-link rather than a. Curious what @andycochran @brettsmason and @JeremyEnglert think about the question on lightness of links inside callouts.

@andycochran
Copy link
Contributor

It would be a pain to have to apply a class to every link inside callouts just to fix their color.

This reminds me of a previous issue: Headers used to have an explicit color. We switched to $header-color: inherit; so that nested components with different background/font colors wouldn't need to each declare specific header colors.

But this is more complicated with links. The nature of cascade works against us here. Since we're applying a specific color to links, a component which needs a different link color must have a rule with more specificity for its links. And this new rule will cascade to all components within. So, if a further-nested component needs a different link color, it must have a rule with even more specificity for its links. And links continue to require greater and greater specificity for each level of nesting.

I'd suggest that we support setting a link color for callouts and note in the docs that "because of the nature of cascade, callouts with nested components require app-specific styling to reset those components' link colors."

@andycochran
Copy link
Contributor

What about using explicit descendant selectors? For example, link colors could be defined for particular descendants in a callout.

@mixin callout-style($color: $callout-background) {
  …
  & > a:not(.button),
  & > p a:not(.button) {
    …
  }
  …
}

This way, nested component won't be effected. No classes necessary.

@ncoden
Copy link
Contributor

ncoden commented Oct 17, 2016

@andycochran

It would be a pain to have to apply a class to every link inside callouts just to fix their color.

This is a well-know problem in BEM, resolved by the Scope classes.

With the solution you propose, links that are direct childs are affected, but links inside a grid (or any other invisible component or utility) are not. Explicit descendant selectors > does not resolve the problem, it moves it.

Also, it keep a specificity of 0011 and does not resolve:

  • a:not(.button) does not ensure that button (or similar components) links are not changed. Buttons can be declared in an other class.

@andycochran
Copy link
Contributor

Good point, @ncoden. I'd like to withdraw my off-the-cuff descendant selectors suggestion.

I suppose we have to make a choice, decide which is a bigger problem:

Option A: Callout Link Classes
Pro: Nested components aren't effected by callout link colors.
Con: Callout links require a specific class to get the appropriate color.

Option B: No Link Classes
Pro: All links in callouts are automatically colored appropriately.
Con: Nested components require app-specific styles.

How common is it to include nested components in callouts? I've never needed to. But I very often have links in my callouts, and I'd hate to have to apply classes to those. If the primary function of callouts is an alert or block of text/links, I'd like to avoid over-engineering a solution that enables callouts to be used for [any unspecified purpose].

@ncoden
Copy link
Contributor

ncoden commented Oct 17, 2016

Option C:

  • Callout Scope Modifier (.callout.callout-links or more strictly .callout.callout--s-links a )
  • Callout Link Classes (.callout .callout-link or more stricly .callout .callout__link)

✓ Pro: Nested components can being not affected by callout link colors.
✓ Pro: All links in callouts can be automatically colored appropriately.


How to make the best Sass approach ever ? Take BEM, SMACSS, ITCSS and mix everything !

Disclaimer: I seen scopes and modifiers before, but never scoping modifiers. Use it at your own risks.

@andycochran
Copy link
Contributor

Can we update this PR to work with the new color-pick-contrast() function and @ncoden's suggestions?

@ncoden
Copy link
Contributor

ncoden commented Dec 3, 2016

Beyond using the BEM element to apply properties precisely, my suggestions introduce a new concept to keep the component simple : scoping modifiers.

  • With scoping modifier for a simple use case: .callout-links a (or .callout--s-links)
<div class="callout callout-links">
  <h5>This is a callout.</h5>
  <a href="#">Link with a color for the callout context</a>
  <a href="#">An other link</a>
</div>
  • With callout link elements to apply properties precisely: .callout-link (or .callout__link)
<div class="callout">
  <h5>This is a callout.</h5>
  <a href="#" class="callout-link">Link with a color for the callout context</a>
  <div class="an-other-context">
    <a href="#" class="callout-link">Link with its own color</a>
  </div>
</div>

@kball I would have your opinion on this.

@kball
Copy link
Contributor

kball commented Dec 5, 2016

I think this concept of a scoping modifier is a potentially a really nice way to walk the balance between control and brevity/ease of use. For folks doing simple things, they can use the scoping modifier and it will "just work" (TM). For folks who want more control, are building complex nesting things, or big apps, they can apply the link element classes. I dig it.

Can we look at some other cases where we might use this concept and see if there are any gotchas lurking?

@ncoden
Copy link
Contributor

ncoden commented Dec 5, 2016

@kball Almost everywhere I yelled against properties applied from the parent. We don't have a high specificity because it's cool to have unmaintainable stuff, but because it permits to have easily usable components (as long as you have a basic usage).

Everywhere there is properties applied from a parent (grid, menu, dropdown, top-bar...), we could switch to a set of modifiers + a scoping modifier.

@wolfgang-demeter
Copy link
Contributor Author

Sorry, i somewhat dropped out of that thread/PR! But i believe you resolve the issue to everybodies satisfaction. 👍

Please don't forget the text color in Callouts!

The reason i stumbled upon this problem is, that we use Callouts in combination with .success, .warning and .alert for messages to the user. In addition we also set $callout-background-fade to 0% to stay within our defined color palette and not genereate various shades of a color. But with a dark red for $foundation-palette(alert) the usual blackish $callout-font-color is very hard to read, if at all! And $callout-font-color-alt - which should resolve that problem - wasn't used at all. The same goes for the link color in Callouts.
To make things more complicated, we offer a SaaS application which is customized via _settings.scss for our customers. Therefore we rely on the fact, that the colors configured there result in a usable/readable CSS file without black text on black backgrounds for example.

@kball
Copy link
Contributor

kball commented Jan 3, 2017

Based on the above conversation, I think we should close this PR and replace it with the scoping-modifiers approach mentioned here: #9098 (comment)

@wolfgang-demeter do you want to take a pass on this? Or @ncoden do you?

@kball
Copy link
Contributor

kball commented Apr 18, 2017

Going to close this based on the above discussion, but would still love it if someone could take a pass on scoping modifiers

@kball kball closed this Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants