Skip to content
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

RFC - add extended grid #1041

Closed
wants to merge 1 commit into from
Closed

Conversation

dashouse
Copy link

@dashouse dashouse commented Oct 24, 2018

Add tablet and desktop specific breakpoints by using the existing grid mixin.

Example:
grid

I would like comments on:

The outputted class names –
<div class="govuk-grid-column-one-third govuk-grid-column--at-desktop-one-quarter"> (We will need to modify the grid mixin if we want to change where the -one-quarter part of the class name is)

If the outputted CSS increases the bundle size too much – Are there any other options?

If it's right to only provide tablet / desktop breakpoint overrides and continue to have mobile default to 100% wide

@dashouse dashouse changed the title DNM - add extended grid RFC - add extended grid Oct 24, 2018
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1041 October 24, 2018 12:17 Inactive
@dankmitchell
Copy link

I like the idea of this but prehaps just have s, m, l would keep the class names shorter and be non-device specific.

@dashouse
Copy link
Author

Hey @dankmitchell - Appreciate the comment :)

At the moment these are linked to the how breakpoints are defined in GOV.UK Frontend everywhere so it's not something I can change for the extended grid alone.

As the SCSS is written mobile first, these breakpoint specific grid classes need to override the default (mobile) declaration. I did originally consider break point specific grid classes for mobile too but there seems to less of a use case for multiple columns at that size. The need we've addressed is really around how things behave on the tablet breakpoint. Replicating how the SCSS is structured my suggestion would be that most of the time you would actually only be defining breakpoint specific classes for desktop as the existing classes would work for mobile and tablet as they do now.

Totally agree about the length of the class names, I thought it might be best to keep the same format as the normal grid classes as they would be used together most of the time. The only part of the name that is fixed is based on the grid-widths map

@36degrees
Copy link
Contributor

Thanks for looking into this Dave – seems like a great addition 👍

In answer to your specific questions:

The outputted class names –
<div class="govuk-grid-column-one-third govuk-grid-column--at-desktop-one-quarter"> (We will need to modify the grid mixin if we want to change where the -one-quarter part of the class name is)

I don't object to the class names as they are, but I think if we were writing this from scratch then swapping the width and the breakpoint (govuk-grid-column--one-quarter-at-desktop) would be more natural?

We could also consider using @ to denote a 'responsiveness' suffix – as suggested here – main advantage IMO would be that it 'stands out' so you can tell from the 'shape' of the class it does something different, without having to read it. But, like our use of the ! prefix, it might need escaping in the CSS?

If the outputted CSS increases the bundle size too much – Are there any other options?

For reference, this adds around 0.1kb (9.113 after - 8.965 before) gzipped or 2.1 kb (77.1 after - 74.976 before) to the dist build. I think that's acceptable… but we could choose to include it as a separate file in the objects folder so that service teams have the option of selectively including only the 'basic' grid without getting these extra classes, if they don't need them.

If it's right to only provide tablet / desktop breakpoint overrides and continue to have mobile default to 100% wide

I guess there's nothing stopping us from adding these later if we see demand for them?

@NickColley
Copy link
Contributor

This seems like a good change, and I'm 👍 to getting it in.

Alternatives could be exploring using CSS Grid for more advanced layouts with a simple fallback to our standard grid.

@dashouse
Copy link
Author

dashouse commented Nov 7, 2018

@36degrees I agree the class name would be better the other way around. Negative of that would be that it's a breaking change to the current grid mixin.

@NickColley After conducting the experiments with CSS Grid and Flexbox options and presenting to the team (you were elsewhere I remember) there was real barriers in terms of how we would actually roll out a CSS Grid based solution. I'd love to do it but this allows the majority of the functionality that people are currently missing without much work.

@NickColley
Copy link
Contributor

We're going to test this proposal in context of the patterns that prompted this RFC, which'll give us the confidence that this solves the problems we think it will.

@36degrees
Copy link
Contributor

I think this has been superseded by #1094@dashouse are you happy for me to close this?

@dashouse dashouse closed this Dec 20, 2018
@dashouse dashouse deleted the add-breakpoint-specific-grid-classes branch December 20, 2018 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants