Simplify the grid mixins #1090
Merged
Simplify the grid mixins #1090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to be consistent with other functions which are namespaced under the govuk prefix. We keep grid-width as a deprecated alias which we can remove in the next major version.
Wrapping the row functionality up in a mixin offers little other than the ability to use a custom name, but we’re not sure there’s a need for users to do that. Simplify the code by deprecating the mixin and just using the code to create the concrete class directly.
The class names generated by the mixin are not flexible enough, as the width is just appended to the class name. In spiking how we might add an extended grid we found the generated class names a little yoda-ish, for example `govuk-grid-column--at-desktop-one-quarter` (bonus points if you actually read that in Yoda’s voice). Moving the class naming out of the mixin and doing it within the objects layer is also more consistent with the rest of Frontend, and means that users who do use the grid mixins directly don’t have to fit with the constraints of our class name structure.
|
I think there would be three issues raised against 3.0 off the back of this work:
|
Nooshu
reviewed
Dec 12, 2018
|
|
||
| $govuk-grid-widths: ( | ||
| one-quarter: 25%, | ||
| one-third: 33.3333%, |
There was a problem hiding this comment.
Quick question. Is it worth letting the Sass language handle the precision here?
Swapping this out for:
one-third: (100/3) * 1%,
I believe this is possible in a map as according to the docs: "Both the keys and values in maps can be any SassScript object".
I doubt it would have any effect on the rounded result so may just be best left hardcoded.
There was a problem hiding this comment.
Might be worth keeping it simple since it works and doesnt require thinking.
NickColley
approved these changes
Dec 12, 2018
There was a problem hiding this comment.
Nice work, this makes the mixin much less complicated while using a regular CSS feature instead of the class option.
I've checked the review application grid page and it renders the regular classes correctly.
👍 for noting we need to open issues to track the deprecations.
https://govuk-frontend-review-pr-1090.herokuapp.com/examples/grid
This was referenced Dec 13, 2018
|
Verified that the CSS output is the same using both snapshots and a quick MD5 check: |
aliuk2012
added a commit
that referenced
this pull request
May 14, 2019
#1090 deprecated this mixin and left it in as an alias to govuk-grid-width. ACTION: Replace all instances of grid-width with govuk-grid-width
aliuk2012
added a commit
that referenced
this pull request
May 14, 2019
#1090 deprecated this mixin and left it in as an alias to govuk-grid-width. ACTION: Replace all instances of grid-width with govuk-grid-width
36degrees
added a commit
that referenced
this pull request
May 23, 2019
This functionality was previously deprecated in v2.5.0, although it was unfortunately not mentioned in the release notes. [1]: #1090
36degrees
added a commit
that referenced
this pull request
May 23, 2019
This functionality was previously deprecated in v2.5.0 [1], although it was unfortunately not mentioned in the release notes. [1]: #1090
aliuk2012
added a commit
that referenced
this pull request
Jun 14, 2019
#1090 deprecated this mixin and left it in as an alias to govuk-grid-width. ACTION: Replace all instances of grid-width with govuk-grid-width
aliuk2012
pushed a commit
that referenced
this pull request
Jun 14, 2019
This functionality was previously deprecated in v2.5.0 [1], although it was unfortunately not mentioned in the release notes. [1]: #1090
aliuk2012
added a commit
that referenced
this pull request
Jun 14, 2019
#1090 deprecated this mixin and left it in as an alias to govuk-grid-width. ACTION: Replace all instances of grid-width with govuk-grid-width
aliuk2012
pushed a commit
that referenced
this pull request
Jun 14, 2019
This functionality was previously deprecated in v2.5.0 [1], although it was unfortunately not mentioned in the release notes. [1]: #1090
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This simplifies the grid to make it easier to add the extended grid.
grid-widthfunction togovuk-grid-widthto be consistent with other functions, retaininggrid-widthas as deprecated aliasgovuk-grid-rowmixin to create a new concrete class and mark the mixin as deprecatedgovuk-grid-columnmixin to allow class name to be suppressed by setting$classtofalse.classparameter of thegovuk-grid-columnmixin as deprecated$govuk-grid-widthsmap.https://trello.com/c/1aBarhHU/1613-add-extended-grid-to-govuk-frontend-pair