Better handle cases where govuk-text-colour is set to a non-colour value
#2573
Conversation
The use of the `rgba` function, which was added to work around a bug in Safari, prevents users from setting `$rgba-text-colour` to inherit. This is useful if you wish to control the text colour by cascading a style down from, say, a body tag, which in turn is useful in order to apply this control at run-time rather than build-time. This fixes the problem by first testing for the value `inherit`. When found we simply evaluate to `inherit`, which is probably the best we can do in the circumstance. When it is not found we apply the `rgba` trick.
Adds some tests for the reinstated support for `inherit` as a `$govuk-text-colour` in the `govuk-link-style-text` mixin.
|
I couldn't find any tests for this mixin so I've tried to add some very simple ones to demonstrate the cases that this PR caters to. When I ran the tests locally, I would get sporadic failures, due to time outs, in random tests. I'm assuming that is a known issue and is nothing to do with these changes. |
There was a problem hiding this comment.
Thanks for picking this up and contributing a fix! I have a suggestion which it'd be good to get your thoughts on.
When I ran the tests locally, I would get sporadic failures, due to time outs, in random tests. I'm assuming that is a known issue and is nothing to do with these changes.
Yep, we are having some issues with flakey tests at the minute – apologies.
We wish to defend against passing `rgba()` a value that is not a colour, such as is the case with `inherit`. Rather than testing for `inherit` specifically it is better to just test the type of the variable to see whether it is a colour or not. We can also avoid setting the `:hover` state entirely which should reduce the size of the resulting CSS. (Thanks to @36degrees for this suggestion.) Co-authored-by: Oliver Byford <oliver.byford@digital.cabinet-office.gov.uk>
The last commit optimised the code to remove the `:hover` state when it isn't required, therefore we must no longer test for its presence.
There was a problem hiding this comment.
Thanks @daniel-ac-martin – this looks good to me. Are you able to add this to the list of unreleased fixes in the changelog? We can do it, if you prefer.
|
@36degrees: How's that? |
daniel-ac-martin
changed the title
|
Thanks @daniel-ac-martin , looks good to me! Just waiting for a second review from someone else in the team, and then this will be merged. |
EoinShaughnessy
changed the title
govuk-text-colour is set to a non-colour value
|
@vanitabarrett: Any chance of a (patch?) release for this? |
|
@daniel-ac-martin the Design System team are working on a couple of things at the moment which might be ready for release soon, so we're going to see how they progress. It may make sense for us to wait and include them in a feature release in the next couple of weeks. |
Was: Reinstate support for 'inherit' as a text colour
The use of the
rgbafunction, which was added to work around a bug inSafari, prevents users from setting
$rgba-text-colourto inherit.This is useful if you wish to control the text colour by cascading a
style down from, say, a body tag, which in turn is useful in order to
apply this control at run-time rather than build-time.
This fixes the problem by first testing for the value
inherit. Whenfound we simply evaluate to
inherit, which is probably the best wecan do in the circumstance. When it is not found we apply the
rgbatrick.
Addresses: #2231