-
Notifications
You must be signed in to change notification settings - Fork 335
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
Change the Button component background and text colour #2752
Conversation
6e886f8
to
8b94554
Compare
427340d
to
8baa05e
Compare
Is it possible to get this considered for triage please? |
Thank you @vanitabarrett ! Appreciate it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of thoughts. Nothing explicitly blocking.
/// @type Colour | ||
/// @access public | ||
|
||
$govuk-button-background-colour: govuk-colour("green", $legacy: #00823b) !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this line be better placed in src/govuk/settings/_colours-applied.scss
? That's where other default variables definitions seem to be located.
I'm conscious that this variable is specific to the button component, however, and we don't seem to have precedent for where to put them specifically. Happy to keep it here if we think that makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking over time I can contribute the different high level component variables based on me branding the GOV.UK Design System so it'll make more sense.
@include govuk-exports("govuk/component/button") { | ||
$govuk-button-colour: govuk-colour("green", $legacy: #00823b); | ||
$govuk-button-colour: $govuk-button-background-colour; | ||
$govuk-button-hover-colour: govuk-shade($govuk-button-colour, 20%); | ||
$govuk-button-shadow-colour: govuk-shade($govuk-button-colour, 60%); | ||
$govuk-button-text-colour: govuk-colour("white"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we're allowing customisation of the background-color, it feels like we should also provide a mechanism to customise the text color so that it's possible to maintain accessible contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this'll also be useful when either I or the GOV.UK Design System team writes about branding. My plan is to contribute these improvements based on real world usage of branding while doing client work. Then I will write guidance on how this is done that could be contributed back to the main GOV.UK Design System if it becomes useful to do so.
8baa05e
to
0a0cf0e
Compare
@querkmachine pushed an update addressing your feedback. |
A commit from your accordion focus colour PR seems to have snuck in and caused a merge conflict. If you can fix that up, I'm happy to approve! |
Ah yeah rouge rebase gone wrong, will sort that thanks. |
0a0cf0e
to
f12e816
Compare
Hi @NickColley, Sorry! This has ended up overlapping with some other work we're doing and we've inadvertently introduced a new merge conflict to your PR — the file at If you're okay to take a look at it, please do. We're currently preparing a bugfix release of Frontend however, so it may be a few more days until this is approved and merged. |
b6c5962
to
01e39bd
Compare
@querkmachine okay sorted that conflict thanks for the heads up. 👍🏻 |
Adds a new `$govuk-button-background-colour` and `$govuk-button-text-colour` public variable inside a new Component / Button section. Allows users who brand GOV.UK to do so without brittle selector overrides which can break in future updates. This variable naming matches other similar variables such as `$govuk-body-background-colour` and `$govuk-canvas-background-colour`.
01e39bd
to
cada8cc
Compare
@NickColley I've just rebased your branch and moved the changelog entry to the right place now that the 4.3.1 release has gone out. Will approve once the tests have ran. Thanks for putting up with the faff! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 🔴 🟡 👍🏻
Thanks @querkmachine @owenatgov for your help getting this one in 👍🏻 |
Expands the public API to include a new
$govuk-button-background-colour
and$govuk-button-text-colour
scss variable.Allows users who brand GOV.UK to do so without brittle selector overrides
which can break in future updates.
This variable naming matches other similar variables such as
$govuk-body-background-colour
and$govuk-canvas-background-colour
.At the moment I have to do this sort of thing:
After: