-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature/design system #352
Conversation
c55a891
to
d209bdd
Compare
Update latest CSS bundle Tweak response feedback spacing
f1e5faf
to
78992a2
Compare
.govuk-header__navigation-list { | ||
margin-top: 0.75rem; | ||
} | ||
@media (min-width: 769px) { |
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 looks different from how GDS do media queries. Is this ok?
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.
That's a good flag @wpfl-dbt. Usually the GDS approach would be better, but as this CSS could also be used outside of Sass I'd suggest this is okay here.
/* BUTTON */ | ||
.govuk-button { | ||
border: 0; | ||
border-radius: var(--border-radius); |
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 have a vague memory that Firefox needs a different solution for border-radius
-- does this needs belt and bracing? Or have times a-changed?
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.
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.
Or is this kind of thing literally why we use Sass?
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.
Firefox should play nicely with border-radius nowadays :-)
Sass doesn't really add anything in this context. I'm building it in a way so it doesn't rely on Sass (even though it's a .scss
file) for compatibility across projects.
@@ -15,7 +15,7 @@ <h1 class="govuk-heading-l">Sign in</h1> | |||
|
|||
<input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}"> | |||
|
|||
<div class="govuk-form-group"> | |||
<div class="govuk-form-group govuk-!-margin-bottom-2"> |
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.
TIL !
is valid CSS
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.
Basically fine with a few queries almost all derived from my unfamiliarity with Sass
LGTM, both code wise and aesthetically! |
Context
Looking at building an overlay for the gov.uk design system, to provide a more bespoke look and feel. This is work-in-progress, but the work so far can be merged.
Changes proposed in this pull request
The way this works