-
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
Simplify grid syntax #665
Simplify grid syntax #665
Conversation
Related - If we go with this we should update |
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 the approach. It feels simpler to me and provides a powerful mixin that others can use in their service if they need a more complex grid.
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 this approach too 🎉👍 Good that users can now define more grid classes if needed. Naming wise, maybe .govuk-grid-col-1-4
(dash instead of slash), it's easier to remember and same as Bootstrap, slash is unusual.
src/globals/objects/_grid.scss
Outdated
// columns. Widths should be defined as fractions of the full desktop width | ||
// they want to fill. By default they break to become full width at tablet size | ||
// but that can be configured to be desktop using the `$full-width` argument. | ||
// |
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.
Hmmm I think $full-width
sounds like 100% width which could be confusing as that's what the mobile column is. Maybe $fraction-width
, $unit-width
... I don't know, naming is hard.
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.
went with $full-width as its the same param as in toolkit. maybe advanced users will be familiar with it.
it's more like "take proportional width at this breakpoint".
$full-width-from
?
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.
Hmmm so the column is not "full width" from that point on. On mobile/tablet the column is full width (or 100% width). At this breakpoint the column width becomes a fraction/unit. So the name of the variable should be the opposite of "full-width" but I don't know what. $fraction-width-from
, $unit-width-from
?
I might not be explaining this very well 😅
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.
$breakpoint?
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 $break-from
? I'm okay with either, we'll learn more in user research.
src/globals/objects/_grid.scss
Outdated
@include govuk-exports("grid") { | ||
// A mixin for a grid column | ||
// Creates a cross browser grid column with a standardised gutter between the | ||
// columns. Widths should be defined as fractions of the full desktop width |
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.
Suggestion: "units" instead of "fractions"
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 units
app/views/examples/grid/index.njk
Outdated
@@ -11,73 +11,97 @@ | |||
|
|||
<main id="content" role="main" class="govuk-main-wrapper"> | |||
<h1 class="govuk-heading-l">Example: Grid layout</h1> | |||
<div class="govuk-grid"> | |||
<div class="govuk-grid__item govuk-grid__item--two-thirds"> | |||
<div class="govuk-layout-row"> |
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 all opinionated but are we sure about layout
? could this instead be grid
? 😁
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 tend to agree, I think govuk-grid-row
/ govuk-grid-column
would be more expected
app/views/examples/grid/index.njk
Outdated
This guide shows how to make your service look consistent with the rest of GOV.UK. It includes example code and guidance for layout, typography, colour, images, icons, forms, buttons and data. | ||
</p> | ||
</div> | ||
<div class="govuk-layout-col-two-thirds on-desktop-three-quarters"> |
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 love the functionality of on-desktop-three-quarters
but I'm worried it doesn't feel like it's part of the grid syntax.
Did we discuss anything like:
govuk-grid-col-two-thirds
d:govuk-grid-col-three-quarters
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.
on-desktop-three-quarters was an example class name. i could have just used
@include govuk-layout-col(three-quarters, $at: desktop, $class: "govuk-grid-col-desktop");
src/globals/objects/_grid.scss
Outdated
@include govuk-layout-col(full); | ||
|
||
// custom 100,66,75% | ||
@include govuk-layout-col(three-quarters, $at: desktop, $class: "on-desktop"); |
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.
Should we have custom examples as part of the core library? It seems good to demonstrate but we shouldn't include them unless we know they'll be used?
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.
no for demo only. still WIP :)
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 looking good to me, and the mixins may make it easier for downstream projects to make custom grids in a supported way.
Nice work 👍
This looks great, very readable/predictable. There was some concern earlier that this was moving away from BEM, is that still the case? |
it's now kinda BEM, just two block elements |
From a quick discussion with @igloosi it seems like In our example page, it doesn't wrap each row, it wraps the whole grid. Is it at all useful to wrap each row? If not maybe the previous name was actually better - just |
i guess if you have wildly differing content length in columns you might want to wrap in multiple rows |
ok cool, so our guidance and examples would be to wrap each row with |
updated the example page |
@joelanman @igloosi I think the choice whether to use grid containers as individual rows or as a wrapper for all grid items is dependent on content to some degree... For example I've used two rows here https://govuk-design-system-production.cloudapps.digital/styles/layout/common-two-thirds-two-thirds-one-third/index.html But equally if you just fill 2/3 1/3 1/4 3/4 it wouldn't technically need a new row as they would wrap. It gets a little more complex when you actually want it to wrap because your grid changes from 4 > 3 > 2 so it doesn't leave widowed columns |
I think even if it would work in some situations if you didn't wrap every row, its an easier thing to get your head around if we always do? |
Sure, so this would be just updating the example in the review app rather than anything else? |
Seems so, I just checked the Design System layout page and it seems like we already wrap every row there. |
and i have updated the review app grid example page as well |
Simplify grid classes based on user feedback
$at
describes when the fraction percentage kicks inDefault usage:
Define when percentage kicks in:
Specify a custom class:
https://govuk-frontend-review-pr-665.herokuapp.com/examples/grid
Note: the column mixin is similar to the Frontend toolkit one
https://github.com/alphagov/govuk_frontend_toolkit/blob/master/stylesheets/_grid_layout.scss#L93
Trello ticket: https://trello.com/c/jjWZkDe0/865-5-simplify-the-grid-syntax