Refactor typography mixin; remove shorthand helpers #772
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
Rather than passing maps around, we want to move to just passing the ‘desktop size’ e.g. `19`, `80` and doing the lookups against the map from within the helper. This will ultimately allow us to reduce the number of mixins we need to provide as part of our public API.
These are being removed in favour of using `govuk-font` throughout - this decouples the codebase from the typography scale (allowing it to be overridden by consumers in the future), reduces the number of mixins in our public API and gives us greater control over being able to e.g. warn if points in the scale are going to be deprecated.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-772
|
looks good. looking forward to the changelog entry :) 📓 |
|
Nice work using the snapshots while developing! |
Changelog entries added!
It worked really well – trying to think of a way we can turn it into a gulp task e.g. |
Merged
36degrees
added a commit
that referenced
this pull request
Jul 5, 2022
As part of #772 we updated the `govuk-typography-responsive` mixin to accept a point from the spacing scale rather than a map (1ea1353, 93a0f33). However the associated SassDocs were not updated at the same time, and still talk about accepting a $font-map param which is incorrect. Update the SassDocs to reflect the way the mixin works. Also update the SassDocs for the `govuk-font` mixin to use the same terminology, and correct ‘as it would appear on desktop’ to ‘as it would appear on tablet’ which is the breakpoint where the font-size actually changes.
36degrees
added a commit
that referenced
this pull request
Jul 5, 2022
As part of #772 we updated the `govuk-typography-responsive` mixin to accept a point from the spacing scale rather than a map (1ea1353, 93a0f33). However the associated SassDocs were not updated at the same time, and still talk about accepting a $font-map param which is incorrect. Update the SassDocs to reflect the way the mixin works. Also update the SassDocs for the `govuk-font` mixin to use the same terminology, and correct ‘as it would appear on desktop’ to ‘as it would appear on tablet’ which is the breakpoint where the font-size actually changes.
36degrees
added a commit
that referenced
this pull request
Jul 12, 2022
As part of #772 we updated the `govuk-typography-responsive` mixin to accept a point from the spacing scale rather than a map (1ea1353, 93a0f33). However the associated SassDocs were not updated at the same time, and still talk about accepting a $font-map param which is incorrect. Update the SassDocs to reflect the way the mixin works. Also update the SassDocs for the `govuk-font` mixin to use the same terminology, and correct ‘as it would appear on desktop’ to ‘as it would appear on tablet’ which is the breakpoint where the font-size actually changes.
querkmachine
pushed a commit
that referenced
this pull request
Oct 17, 2022
As part of #772 we updated the `govuk-typography-responsive` mixin to accept a point from the spacing scale rather than a map (1ea1353, 93a0f33). However the associated SassDocs were not updated at the same time, and still talk about accepting a $font-map param which is incorrect. Update the SassDocs to reflect the way the mixin works. Also update the SassDocs for the `govuk-font` mixin to use the same terminology, and correct ‘as it would appear on desktop’ to ‘as it would appear on tablet’ which is the breakpoint where the font-size actually changes.
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.
The shorthand helpers are being removed in favour of a new mixin
govuk-fontthroughout - this decouples the codebase from the typography scale (allowing it to be overridden by consumers in the future), reduces the number of mixins in our public API and gives us greater control over being able to e.g. warn if points in the scale are going to be deprecated.Rather than passing font maps around, pass the 'key' from the scale instead.
These changes have been made whilst running a Jest test that expects against against a known-good snapshot of both the default and IE8 stylesheets from master, so the output should be exactly the same.
https://trello.com/c/B8OCSU8R/759-agree-on-and-document-public-api-for-govuk-frontend