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

Refactor spacing API #779

Merged
merged 7 commits into from
Jun 13, 2018
Merged

Refactor spacing API #779

merged 7 commits into from
Jun 13, 2018

Conversation

hannalaakso
Copy link
Member

@hannalaakso hannalaakso commented Jun 12, 2018

This PR:

  • Defines the spacing variables in a map, rather than as multiple variables, allowing control over error messaging when an incorrect variable is used and allows them to be overridden by consumers.

In detail, it:

  • Adds a new helper govuk-spacing to fetching single point spacing.
  • Moves single point spacing variables inside a map.
  • Moves responsive spacing maps inside a collective map $govuk-spacing-responsive-scale and updates the helper.
  • Removes the old $govuk-spacing-responsive-scale map we created for generating overrides as the overrides can now use the new scale map 🎉
  • Possibly controversial: makes govuk-responsive-spacing() a private mixin as it should only be used via govuk-responsive-margin and govuk-responsive-padding.
  • Updates other layers to use the govuk-spacing function and the new responsive spacing map.
  • Updates tests accordingly and adds new tests for govuk-spacing()
  • Updates comments for sass-docs.

Other:

  • $govuk-gutter is now hardcoded as govuk-spacing cannot be accessed from inside the settings layer. However if we think it should be coupled more tightly with the spacing scale, we could maybe use map-get to fetch a specific spacing point in settings... I’ve created an issue to review this.

Note: the PR is easiest to review commit-by-commit.

https://trello.com/c/0IDTgRtF/1112-provide-a-defined-public-api-for-govuk-frontend-spacing

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-779 June 12, 2018 07:58 Inactive
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Raised a few very minor consistency things, but otherwise this looks really good – great commit structure and messages 👌

///
/// These definitions are used to generate responsive spacing that adapts
/// according to the breakpoints (see 'helpers/spacing'). These maps should be
/// used wherever possible to standardise responsive spacing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picky, but can we add the /// to the blank lines to so that it reads as one connected comment block?

///
/// @param {Map} $scale-map - Map of breakpoints and spacing values
/// @param {Number} $responsive-spacing-point - Point on the responsive spacing
/// scale, corresponds to a map of breakpoints and spacing values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to indent the second line like we have on :48

+ "responsive spacing scale.";
$actual-input-type: type-of($responsive-spacing-point);
@if $actual-input-type != "number" {
@error "Expected a number (integer), but got a "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is short enough it'll fit on one line?

/// @param {String} $direction [all] - Direction to add spacing to
/// (`top`, `right`, `bottom`, `left`, `all`)
/// @param {Boolean} $important [false] - Whether to mark as `!important`
/// @param {Number} $adjustment [false] - Offset to adjust spacing by
///
/// @example scss
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@@ -67,7 +110,7 @@ describe('@mixin govuk-responsive-spacing', () => {
padding-top: 25px; } }`)
})

it('throws an exception when passed anything other than a map', async () => {
it('throws an exception when passed anything other than a number', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically '14px' is a number in Sass, so maybe 'throws an exception when passed a non-existent point' or similar?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-779 June 12, 2018 09:39 Inactive
Put single spacing points in a map. Put responsive spacing maps inside
a collective map.

Remove old `$govuk-spacing-responsive-scale` map as the overrides can
now use the new scale map :)

Update comments for sass-doc.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-779 June 12, 2018 09:58 Inactive
// You can define different behaviour on tablet and desktop. The 'null'
// breakpoint is for mobile.
$govuk-spacing-points: (
0: 0,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in there any point in having 0 in teh scale, since we've used 0 where needed instead of `govuk-spacing(0)?

.govuk-header--full-width {
   padding: 0 govuk-spacing(3);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because it means there's an override generated for it, so you can remove margins.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one more thing I didn't spot first time round. 👀

/// @param {String} $property - Property to add spacing to (e.g. 'margin')
/// @param {String} $direction [all] - Direction to add spacing to
/// (`top`, `right`, `bottom`, `left`, `all`)
/// @param {Boolean} $important [false] - Whether to mark as `!important`
/// @param {Number} $adjustment [false] - Offset to adjust spacing by
///
/// @access public
/// @access private
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is private then the mixin name should be prefixed with an underscore (_govuk-responsive-spacing). Change makes sense I think as you can achieve everything with govuk-responsive-margin and govuk-responsive-padding?

@mixin govuk-responsive-margin($scale-map, $direction: "all", $important: false, $adjustment: false) {
@include govuk-responsive-spacing($scale-map, "margin", $direction, $important, $adjustment);
@mixin govuk-responsive-margin($responsive-spacing-point, $direction: "all", $important: false, $adjustment: false) {
@include govuk-responsive-spacing($responsive-spacing-point, "margin", $direction, $important, $adjustment);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could pass all arguments like we do for mq if we wanted to avoid having to maintain the same signature in three places (though we'd need to update the docs anyway I guess…)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank @36degrees. As discussed IRL, we need to pass the property (margin / padding) through to _ govuk-responsive-spacing so $args... wouldn't work in this scenario.

All helpers now use single point spacing which is used to fetch maps
from `settings/spacing`. Responsive helper now uses `map-get` to fetch
spacing map, instead of accessing the variables directly.

Add error checking.

Update comments for sass-doc.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-779 June 13, 2018 08:26 Inactive
@@ -23,28 +23,30 @@ $spacing-directions: (
// .govuk-\!-mt-r1 {
// margin-top: [whatever spacing point 1 is...]
// }
///
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the rest of this code block be updated to Sassdoc? (triple slash, use @example, document the parameters)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, good shout @36degrees. Done now.

Point from spacing scale is now used to generate overrides, instead of
passing the map through directly.

Also prefix `generate-spacing-overrides()` with `govuk-`.
Add tests for new `govuk-spacing` function
Use `govuk-spacing()` instead `$govuk-spacing-*'`

`$govuk-gutter` is problematic as it cannot call `govuk-spacing()` helper
from the `settings` layer. We *think* that the gutter variables can be
decoupled from the spacing scale so this commit hardcodes them. However
there's now a GitHub issue added to review this relationship.
Update `govuk-responsive-margin` and `govuk-responsive-padding` to
use new spacing maps.
/// @param {String} $property-shorthand - Shorthand representation of property, eg. "m" for "margin"
///
/// @example scss
/// .govuk-\!-m-r0 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a note: this will need to be updated in #786

Copy link

@kr8n3r kr8n3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy it address all comments

💯

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@hannalaakso hannalaakso merged commit 5198d71 into master Jun 13, 2018
@hannalaakso hannalaakso deleted the spacing-api branch June 13, 2018 15:30
@hannalaakso hannalaakso mentioned this pull request Jun 14, 2018
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.

4 participants