Make override classes consistently verbose #786
Merged
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
kr8n3r
reviewed
Jun 13, 2018
CHANGELOG.md
Outdated
| @@ -153,6 +153,27 @@ Note: We're not following semantic versioning yet, we are going to talk about th | |||
|
|
|||
| ([PR #778](https://github.com/alphagov/govuk-frontend/pull/778)) | |||
|
|
|||
| - Make override classes consistently verbose #786 | |||
|
this needs updating |
|
yaml and by extension readme files in radios |
Merged
We've extended the shorthand 'f' to longhand 'font-size' and extended the shorthand 'w' to longhand 'font-weight' This changes the selectors from `.govuk-!-f-rN` to `.govuk-!-font-size-N`.
861b2a2 to
d9399d6
Compare
d9399d6 to
acd6638
Compare
|
For reviewers, I've been running the following in the review application var stylesheet = document.styleSheets[0]
var selectors = []
var rules = stylesheet.cssRules;
for (var i = 0; i < rules.length; i++) {
selectors.push(rules[i].selectorText);
}
selectors =
selectors
.filter(selector => selector)
.filter(selector => selector.startsWith('.govuk-\\!'))
.map(selector => selector.trim())
console.log(selectors.join('\n'), selectors.length)To check the before and after. |
acd6638 to
74052a3
Compare
hannalaakso
reviewed
Jun 13, 2018
src/overrides/_spacing.scss
Outdated
| @@ -23,40 +23,39 @@ $_spacing-directions: ( | |||
| /// indicates that. We might want to add non-responsive ones later. | |||
There was a problem hiding this comment.
The bit about "the 'r' before the scale point indicates that" just needs to come out.
There was a problem hiding this comment.
Good spot thanks Hanna
We've made the decision to remove the 'r' for responsive, this was hard to remember users found this confusing. We'll consider adding a '--fixed' modifier in the future if we require a static version of this override.
8bd452e to
6c84380
Compare
Merged
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.
In https://gist.github.com/nickcolley/f135e89ed4b679355b0ab47135b38ee8 I went over the current inconsistencies with our override classes.
The community was in favour of making these consistently verbose.
We've extended the shorthand 'f' to longhand 'font-size' and
extended the shorthand 'w' to longhand 'font-weight'
This changes the selectors from
.govuk-!-f-rNto.govuk-!-font-size-N.We've made the decision to remove the 'r' for responsive, this was hard to remember users found this confusing. (Closes #689)
We'll consider adding a '--fixed' modifier in the future if we require a static version of this override.
Override classes prior to this change (total of 118):
Override classes after this change (total 118):