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

Stop specifying black colour for plain elements and basic components #954

Closed
frankieroberto opened this issue Aug 10, 2018 · 4 comments
Closed

Comments

@frankieroberto
Copy link
Contributor

Currently, lots of elements, including a basic p with no class, and classes like govuk-list, set an explicit foreground colour of black.

This means that if you include these elements within a container which specifies a different background and foreground colour (say, white on blue), you have to override the colours of all the elements within it, eg:

.my-container {
  color: govuk-colour("white");
  background-color: govuk-colour("blue");

  p, a, .govuk-list, .govuk-heading-s {
    color: inherit;
  }
}

...this seems a tad unexpected, and makes for harder to maintain code (as you keep having to add to the list of things which should inherit the colour whenever the content changes). My expectation was that the black foreground colour (and white background colour) would be set on a high-level container element (like .govuk-template__body), and other black text would just inherit from that.

However, perhaps there’s a reason why you’ve implemented it this way?

Either way, I figured it'd be useful to have the discussion... 😄

@NickColley
Copy link
Contributor

NickColley commented Aug 10, 2018

Hey Frankie,

We intend for GOV.UK Frontend to work the best it can with older projects for example GOV.UK Elements, or users' custom CSS.

With this in mind we try to avoid relying on global styles where possible.

For the p and a elements those require an opt in global-styles variable.

In your example you seems to be doing inverse styling which is not something we've properly explored yet.

Does that answer your question?

@frankieroberto
Copy link
Contributor Author

@NickColley I'm using the latest version of the Prototyping Kit, and that seems to have global styles enabled by default? (at least, p seems to be styled).

I appreciate the desire not to interfere with existing CSS, however I don't think that removing the foreground colour from

@include govuk-text-colour;
and specifying it here:
background-color: $govuk-body-background-colour;
instead would have any adverse affects? Plus it would be specifying the foreground colour in the same place as the background colour, which is often recommended from an accessibility perspective (as it makes it less likely that someone might override one but not the other).

I guess ultimately what I'm trying to understand is the best way override the default foreground and background colours for a component, whilst keeping the other properties such as font size, weight and spacing. At the moment this feels tricky as both typography and colour are specified together, so you have to resort to using the color: inherit; hack from my example...

@NickColley
Copy link
Contributor

I appreciate the desire not to interfere with existing CSS, however I don't think that moving the foreground colour would have any adverse affects?

Potentially, I'm not sure this would be a problem for new projects but we'd need to be really careful making this kind of change.

often recommended from an accessibility perspective (as it makes it less likely that someone might override one but not the other).

We've found that overriding colours is not dependant on the place where the colours are set.

I guess ultimately what I'm trying to understand is the best way override the default foreground and background colours for a component

For this specific case, the approach you're taking looks sound.

If you're extending a component such as a button I would try something like:

<div class="panel panel--inverse">
    <button class="button button--inverse">I'm an inverse button</button>
</div>

For links, you'll want to setup custom logic since you'll need to handle the different hover focus active states.

For the rest you could consider a helper class, or stick with what you have in your snippet above.

I think we could do a better job of making this easier by doing more work around making the panel component more generic, which Tim has mentioned on the backlog: alphagov/govuk-design-system-backlog#55 (comment)

@NickColley
Copy link
Contributor

I'm going to close this out as it's pretty old, but if this is causing problems for you please re-open, thanks @frankieroberto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants