-
Notifications
You must be signed in to change notification settings - Fork 338
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
[DO NOT MERGE] Experiment with toggling the header colour via a top-level class in the review app #5746
base: main
Are you sure you want to change the base?
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 679c6c6 |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
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 like it! 😍 It makes it super easy to swap between the two colours. With a little update to how we handle the request for swapping in the review app, I think we could even accommodate Percy (see comment on the middleware).
We'd probaby want to keep an eye on the extra CSS to see that it doesn't snowball too much. If it does, having that specific class regrouping all the changes will help us extract the extra CSS for the recoloured version in its own file should we want to, so I'm not too worried.
packages/govuk-frontend-review/src/common/middleware/recolour.mjs
Outdated
Show resolved
Hide resolved
packages/govuk-frontend-review/src/common/middleware/recolour.mjs
Outdated
Show resolved
Hide resolved
padding-bottom: govuk-spacing(2); | ||
border-bottom: $govuk-border-width-wide solid #fff500; | ||
} | ||
|
||
.govuk-feature--recolour .app-header--campaign { |
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 like how having a global class enabling the feature allows us to easily update the app code to accomodate the flag (though I'd guess in a service, you'd either have or not have the flag enable so would need that less so).
packages/govuk-frontend-review/src/stylesheets/full-page-examples/campaign-page.scss
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/header/_index.scss
Outdated
Show resolved
Hide resolved
b90bbe3
to
fba6572
Compare
fba6572
to
cd3b5af
Compare
cd3b5af
to
679c6c6
Compare
Change
Replicate the changes to the header in #5724 but instead of a sass variable, use a class
govuk-feature--recolour
to control the style changes instead.This additionally deviates from #5724 by experimenting with an alternative method of swapping between using the 'old' and 'new' colours in the review app by using a cookie, controlled via middleware, similar to how the banner operates. This also adds an extremely bare bones UI for swapping between 'old' and 'new'.
Notes
Comparing the stats comment on this PR and the stats comment on #5742, this change adds roughly 1.5kb to the CSS. That feels acceptable to me but I'm interested in what others think. Additionally, I suspect this is better than having a class localised to the header because it would mean any other work we do on other components will need their own classes, which theoretically even more kbs compared to just controlling them via the same class (unverified opinion).
Something I want to try is experimenting with mergin the approach from this PR and #5742 where I only make the new class available in the CSS when a sass variable is set. This would enable us to control the weight of the css whilst theoretically maing our own css less complicated.
I also still haven't figured out how to record these different states in Percy. I'll investigate this soon.