Simplify main wrapper spacing logic #1371
Conversation
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1371
src/objects/_main-wrapper.scss
Outdated
| // </div> | ||
|
|
||
|
|
||
| @mixin govuk-main-wrapper { |
There was a problem hiding this comment.
Maybe I'm being too trigger happy and we should just leave these, I'm just not sure why they're part of the public api.
There was a problem hiding this comment.
Yeah, I'm not sure either…
By the looks of it, I added the mixins in 4c01202 but I've no idea why.
src/objects/_main-wrapper.scss
Outdated
|
|
||
| .govuk-main-wrapper--l { |
There was a problem hiding this comment.
We could in theory leave this modifier around, there might be cases where it could be useful?
My reasoning for removing it is, that this modifier should only be applied when the main wrapper is the first child. cc @dashouse is this correct?
There was a problem hiding this comment.
I guess there might be edge cases where for reasons a service ends up with an empty or non-visible element before the main wrapper? For example, a script tag, or an empty container that's always there even if there's no phase banner / breadcrumbs / whatever.
I think I'd be tempted to keep the class just so that there's an option available in those cases?
There was a problem hiding this comment.
Based on existing implementations of Alerts (alphagov/govuk-design-system-backlog#2) I would second keeping the class
There was a problem hiding this comment.
Makes sense to me lets keep it.
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1371
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1371
|
We could consider doing this after v3.0.0 and deprecating things rather than adding this as a breaking change, but I figured the risk was low since migration is automatic? |
NickColley
force-pushed
the
css-only-before-content
branch
from
f452880 to
f716608
Compare
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-1371
|
Could get this into 2.12.0 if we were so inclined... |
We're not sure these are useful, so these will be removed in a future release, if you are using this please let us know.
By using :first-child we can avoid the need for a modifier class, which is often missed. We may be able to simplify the documentation based on this change.
NickColley
force-pushed
the
css-only-before-content
branch
from
f716608 to
c40f863
Compare
By using a
:first-childselector we can avoid the need for a modifier class, which is often missed.This also removes the mixins and modifier class, as there's no clear need for these things.
This work would also allow us to simplify the guidance around main wrapper, since the modifier class is no longer needed: https://design-system.service.gov.uk/styles/layout/#page-wrappers
You can compare pages, which used to require the modifier class which no longer do.
Closes #888
Update: I've changed the approach based on the feedback so it's not a breaking change.
It includes deprecations instead tracked by this issue: #1379
We still may want to update the guidance but it might not be as drastic: alphagov/govuk-design-system#887