-
Notifications
You must be signed in to change notification settings - Fork 324
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
Add ability to invert background colour in review app examples #3808
Conversation
6ee209c
to
138e81e
Compare
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.
This is brilliant 🙌
Can you fix the "inverse" colour for $app-preview-border-colour
too?
We're using #e4f2ff
at the moment which only looks good on a white background, maybe pair it with:
$app-preview-border-colour: mix(govuk-colour("blue"), govuk-colour("white"), 10%);
$app-preview-border-colour-inverse: mix(govuk-colour("white"), govuk-colour("blue"), 10%);
Added a comment about background colour classes too
138e81e
to
9261f96
Compare
packages/govuk-frontend/src/govuk/components/button/button.yaml
Outdated
Show resolved
Hide resolved
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 going to approve this because it's looking amazing since the last push
Added some thoughts on invertLayoutColours
naming but not a blocker 😊
9261f96
to
7401164
Compare
7401164
to
d6b59c0
Compare
@@ -131,10 +131,15 @@ export default async () => { | |||
{{ ${macroName}(${macroParameters}) }} | |||
`, {}) | |||
|
|||
let bodyClasses = '' | |||
let bodyClasses = 'app-template__body' |
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.
Not a blocker but now I've seen layoutModifiers
should we use an array here too?
Could swap +=
with .push()
then have a .join(' ')
as it passes into the render context
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 gave it a shot at one point, but setting the component-preview
modifier by pushing it into an array just... didn't work as expected for reasons I couldn't understand. It was being applied in contexts where it shouldn't on first load, but then would work correctly after restarting the server (and then break again..., rinse, repeat).
The res.render
line didn't like having the .join
inline either, so I just kept it as-is rather than try and debug and optimise where it might not be needed.
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.
Still superb, approving again 🙌
…port Add ability to invert background colour in review app examples
Adds support for adding an array of modifiers (
previewLayoutModifiers
) to review app examples. If the array exists, the example is rendered with the provided modifier classes.Resolves #3780.
Changes
previewLayoutModifiers
array in each example's metadata. If it finds it, it loops through it and adds an additional modifier class to thebodyClasses
string..app-template__body--inverse
class. It makes the body background blue..app-whitespace-highlight
.<html>
/<body>
to follow a similar scheme to the Frontend template:.app-no-canvas-background
to.app-template
..app-template__body
class to the<body>
element by default..app-iframe-in-component-preview
to.app-template__body--component-preview
.Thoughts
<body>
when they might make more sense being on<html>
. However, thehtmlClasses
Nunjucks variable doesn't seem to be accessible at the same point thatbodyClasses
is being set.