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

Add automatic vertical spacing modifier for main wrapper #1493

Merged

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Jul 12, 2019

We have found relying on :first-child alone for automatic spacing can be a a breaking change.

So we have changed this implementation slightly to be opt-in.

We intend to change the guidance (non-blocking v3) alphagov/govuk-design-system#887, users can then remove this class easily if they do not need the custom spacing.

Closes #1479

Previous pull request: #1371

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1493 July 12, 2019 11:42 Inactive
@NickColley NickColley marked this pull request as ready for review July 12, 2019 11:44
@NickColley NickColley added this to the v3.0.0 milestone Jul 12, 2019
CHANGELOG.md Outdated
@@ -408,14 +408,14 @@ changelog](./docs/contributing/versioning.md#updating-changelog).

([PR #1367](https://github.com/alphagov/govuk-frontend/pull/1367))

- Simplify `.govuk-main-wrapper` logic to avoid the need for large modifier in most cases.
- Add new main wrapper modifier to avoid the need for large modifier in most cases.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change note will be removed so I've just done the bare minimum to make this history make sense.

@NickColley
Copy link
Contributor Author

NickColley commented Jul 15, 2019

I talked to Mark and we think to make the 'opt-in' --auto-spacing modifier worthwhile we should update every example in the Design System website containing the main wrapper to use the modifier class, then the guidance will explain what to do if you need to remove it.

@36degrees my gut feeling is that this feels really clunky and I'm had me double guessing if we should have this additional class, maybe it is OK for people like Pay to have to override the unwanted spacing since they need to have custom CSS classes anyway.

Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -57,7 +57,7 @@
// spacing without the need for the `l` modifier, but it is available in
// instances where there may be empty elements that would be difficult to
// remove.
.govuk-main-wrapper:first-child,
.govuk-main-wrapper--auto-spacing:first-child,
Copy link
Member

@hannalaakso hannalaakso Jul 16, 2019

Choose a reason for hiding this comment

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

I think it might help to clear up some of the confusion if we expand on the comment to say that this bit of code ensures the correct margin when there are no other elements like breadcrumbs or back link above the main container

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 the comment is slightly out of date as it still talks about the :first-child as if it's always applied.

What about:

  // Using the `govuk-main-wrapper--auto-spacing` modifier should apply the
  // correct spacing depending on whether there are any elements (such as a back
  // link or breadcrumbs) before the `.govuk-main-wrapper` in the 
  // `govuk-width-container`.
  //
  // If you need to control the spacing manually, use the
  // `govuk-main-wrapper--l` modifier instead.

?

@NickColley NickColley changed the title Change automatic main wrapper behaviour to modifier Add automatic vertical spacing modifier for main wrapper Jul 22, 2019
@NickColley NickColley force-pushed the change-automatic-main-wrapper-behaviour-to-modifier branch from 83cddbf to 6ac9180 Compare July 22, 2019 11:57
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1493 July 22, 2019 11:58 Inactive
CHANGELOG.md Outdated
@@ -298,13 +298,17 @@ You can now add attributes like classes, rowspan and colspan to table row header

[Pull request #1367: Allow for classes, rowspan, colspan and attributes on row headers](https://github.com/alphagov/govuk-frontend/pull/1367). Thanks to [edwardhorsford](https://github.com/edwardhorsford).

### Use page wrappers without a modifier
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@m-green this updates how the page wrapper works

Copy link
Contributor

@m-green m-green left a comment

Choose a reason for hiding this comment

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

Some minor tweaks from me - mainly to make sure we match the language in https://design-system.service.gov.uk/styles/layout/.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1493 July 22, 2019 13:26 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

'Automatic' main-wrapper margin may be applied incorrectly in some situations
5 participants