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

Improvements to inverse components #3925

Closed
wants to merge 2 commits into from

Conversation

querkmachine
Copy link
Member

Makes some improvements to how we handle colours for 'inverse' components, which are intended to be used on dark background colours.

Includes the changes discussed in #3918.

Changes

  • Adds a global variable to define the inverse text colour: $govuk-inverse-text-colour.
  • Changes the govuk-link-style-inverse mixin to use the new variable.
    • This bubbles the change to the Back Link and Breadcrumbs components, which use the mixin.
  • Changes the Breadcrumbs component to use the new variable (for arrows and plain text breadcrumb items).
  • Changes the default colours used by the inverse Button:
    • The text colour has been changed from govuk-colour("blue") to $govuk-brand-colour.
    • The background colour has been changed from govuk-colour("white") to $govuk-inverse-text-colour.
  • Refactors the Button component so that the inverse button colours can be changed manually if desired, e.g. if the inverse text colour and brand colour do not have sufficient contrast with one another.
    • These are exposed as $govuk-inverse-button-text-colour and $govuk-inverse-button-background-colour.

Not changed

  • There are several other components where we use white text on a coloured background, but I have not changed them to use the new variable. This is because the background colours are hardcoded in many of these instances, so a user could not easily change them to complement a modified $govuk-inverse-text-colour. This includes:
    • Header — hardcoded white on black
    • Panel (confirmation) — hardcoded white on green
    • Notification Banner (success) — hardcoded white on green
    • Warning Text — icon is hardcoded white on black

Thoughts

  • Interestingly, the default Notification Banner is hardcoded to use white on $govuk-brand-colour, despite there being no guarantee that the brand colour provides sufficient contrast. This seems like it could be an oversight. Does it need looking into?

@querkmachine querkmachine self-assigned this Jul 7, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3925 July 7, 2023 14:02 Inactive
@querkmachine querkmachine requested a review from a team July 11, 2023 17:15
@paulrobertlloyd
Copy link
Contributor

Interestingly, the default Notification Banner is hardcoded to use white on $govuk-brand-colour, despite there being no guarantee that the brand colour provides sufficient contrast.

I noticed this too. It may also cause issues if the brand colour is the same or similar to another notification banner or error colour (blue, green or red). For example, in the MoD Design System, the information banner also has a reddish colour:

Notification banner component for the MoD Design System

@querkmachine querkmachine force-pushed the inverse-colour-improvements branch from f76954a to ef9d72f Compare July 24, 2023 15:55
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3925 July 24, 2023 15:56 Inactive
@querkmachine querkmachine marked this pull request as ready for review July 24, 2023 16:14
@colinrotherham
Copy link
Contributor

Would you mind giving this a review please @paulrobertlloyd?

Especially with your colour variations suggestion in mind #3918 (comment)

@querkmachine Shall we get designer feedback on the hard-coded things?

@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Jul 27, 2023

Thanks for the prompt @colinrotherham. After plentiful yak shaving, I have updated my prototype masthead component, and added an option to set a value for $govuk-inverse-text-colour in the GOV.UK Eleventy Plugin.

Playing with these 2 values, I foresee an issue, namely this:

Screenshot of the header and masthead components. The dark grey inverse text is visible when set above the light grey brand colour, but not when set on the black header background colour.

What does ‘inverse’ mean? Does it mean:

text that appears on a known dark background colour

or does it mean:

text that appears on the brand colour

I believe the intention is the former, but in the examples provided so far, the later.

Ah themeing, it's a big ’ol bag of worms! I foresee 2 options:

  1. $govuk-inverse-text-colour is intended to be used for text whose background colour is known to be dark given the value is defined within govuk-frontend.

    It’s up to consuming libraries to decide how and when to use this value (I think the masthead component should probably not use it directly, but maybe have it as a default value instead)

    TL;DR: Ship it!

  2. Add an explicit govuk-brand-text-colour (or similarly named) variable in addition or instead of the above, to be used when text is intended to appear upon the unknowable brand colour. Right now, I’m not sure any components in govuk-frontend have that concept.

    TL;DR: Reconsider approach, deciding whether to add another global variable or change the behaviour/intention of $govuk-inverse-text-colour.

@querkmachine
Copy link
Member Author

"Text on a known background colour" is probably closest to how I've been imagining it, though not necessarily a known dark background colour. If the brand colour is quite light, then the inverse colour will probably be darker.

But then that makes me think that you're right in that it's more practically an inverse of the brand colour, other than a universal constant. That might be the safer assumption to make, here.

@querkmachine
Copy link
Member Author

My thinking currently is to:

  1. Maintain $govuk-inverse-text-colour as being for contrast on dark backgrounds. (Default: white).
  2. Create a new global variable called $govuk-brand-text-colour, which is explicitly for contrast with the brand colour. (Default: white).
  3. Update the inverse button's default styles to use $govuk-brand-text-colour.
  4. Do not change the inverse link helper, breadcrumbs or back link (continue using $govuk-inverse-text-colour).

This might feel a little redundant, as $govuk-brand-text-colour will only be used in one place, but the semantic distinction feels kind of important. Even if we don't use it much internally, it'll probably be useful to users of Frontend who are customising colours.

The helper, breadcrumbs and back link are a bit harder to pin down. Although they are likely to be used on the brand colour, there isn't a guarantee that they will. We use the inverse link style on red in our review app, for example.

If I may be so bold as to nullify my own work, there may be more value in only making the changes to allow the button colour customisation and to save the rest until we have a more comprehensive idea on how we want to implement theming.

@paulrobertlloyd
Copy link
Contributor

paulrobertlloyd commented Jul 27, 2023

A few different use cases and concepts are getting combined here:

  • Text that appears on dark background
  • Text that appears on the brand colour

…with an added complication that the inverse button is itself inversely coloured 🤪

For the former, perhaps it makes sense to continue using private variables or hard-coded values (i.e. the header text should be hard-coded to use white because the header background has been hard-coded to use black). Does $govuk-inverse-text-colour need to be a global variable?

I think the later is the use case these inverse components best address, for which there are plentiful examples in the wild, and where this formalisation would help. Having a $govuk-brand-text-colour global variable to compliment $govuk-brand-colour makes a lot of sense, but less clear is how it is applied to different components and helpers. You’re right, there may be cases where ‘inverse’ !== ‘on the brand colour’.

This is making my head hurt a bit. There’s quite a few hypotheticals being thrown into the mix, too - helpful to test robustness of approach, but perhaps a case of great being the enemy of good?

Maybe this is something to get some other (fresher) eyes on? 👀

@querkmachine
Copy link
Member Author

querkmachine commented Aug 7, 2023

Having given time for others on the team to contribute if they so wished, and having mulled this over a little, I think this PR is going to be closed in favour of #4043 for the time being.

Over the last few weeks we've had a lot of questions around theming, implementing dark modes, and deeper customisation of Frontend. In my mind it makes sense to tackle all of those together, but the team is already in the midst of radical architecture changes and refactoring for our upcoming v5 release, and I don't think we want to add this to the bucket at this time, even if this PR represents only a small part of a greater vision.

I'd really like to revisit this in future, potentially during or after we migrate to the Sass module system (#1791), as the way that works really lends itself well to theming and customisation.

This isn't a 'no', just a 'not yet'. Hopefully #4043 resolves the most pressing issue with the inverse button component!

@paulrobertlloyd
Copy link
Contributor

Understandable; once you start digging into the weeds of this, it gets complicated very quickly, and lots of assumptions and unknowns come in to play. Hopefully #4043 improves things, if only by a tiny bit, and having the new inverse components in the first place is already a massive step forward. Thanks for taking the time to noodle on this ❤️

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

Successfully merging this pull request may close these issues.

4 participants