Options for adding extra vertical spacing between blocks #4296

matthillco · 2024-10-18T15:11:47Z

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

What

Two new ways to add extra spacing between blocks:

a generalised class for adding a pre-defined responsive bottom margin on any block
new options for govspeak blocks to add top and bottom static margins

The numbers to pass to the govspeak margin options are taken from the Design System Spacing guide and range from 0-9. It's currently using the static margins, but this could instead use the responsive margins depending on which works best for this use case: might need design involvement to determine static or responsive.

Why

Blocks for landing pages do not currently match the landing-page designs and need increased breathing space.

Trello

andysellick

Took me several comments to get to my overall thoughts about this approach - probably best to just skip to the last comment first 👍

andysellick · 2024-10-21T08:43:46Z

app/assets/stylesheets/views/_landing_page.scss

+// govuk-spacing(x) helper does not include large
+// enough sizes.
+
+.app-margin-bottom-responsive.app-margin-bottom-responsive  {


Instead of trying to force this to override the existing margin bottom on each block, why not just remove the margin bottom from each block and let this class handle it?

andysellick · 2024-10-21T08:44:15Z

app/assets/stylesheets/views/_landing_page.scss

+// govuk-spacing(x) helper does not include large
+// enough sizes.
+
+.app-margin-bottom-responsive.app-margin-bottom-responsive  {


This class name is a bit verbose, maybe just block-margin?

Having app in the name is prudent since it helps identify where the style is coming from: that's useful for other developers who might be looking at the markup to obtain clues for where to find associated styles. I'd like to avoid the word block here because it might cause confusion over whether we're talking about the blocks of content or block level margins. On balance then, I think the original class name I proposed makes more sense.

Using "app" is not a pattern we use anywhere but components, though, let's avoid that.

OK, understood. I think I'll revisit this once I've nailed the approach: if we end up using a mixin we might not need this named class at all.

andysellick · 2024-10-21T08:47:15Z

app/views/landing_page/blocks/_govspeak.html.erb

@@ -1,3 +1,18 @@
+<%-
+  margin_top = block.data["margin_top"]


When do we need margin top on the govspeak block?

It seemed prudent to offer it as an option and as a corollary to having the bottom margin. I understand it's possibly an example of solving a problem we don't yet have, happy to remove it in that case.

In that case let's remove it - we usually try to have a margin down approach.

andysellick · 2024-10-21T08:48:09Z

app/views/landing_page/blocks/_govspeak.html.erb

+
+<% if classes.present? %>
+  <div class="<%= classes %>">
+<% end %>


Two ifs like this is a bit messy - better to either use a content_for or just wrap the govspeak in a div that may or may not have classes.

OK. Would the div still need a class name to identify it, or would it just end up as unnamed in the markup if there are no margin classes on it? It seems a bit wrong to have unnamed divs in the markup as that doesn't really fit the BEM model.

Sorry, I meant to refer to capture there. It's something like...

<% markup = capture do %> (the markup you're creating) <% end %> <% if classes.present? %> <div class="<%= classes %>"> <%= markup %> </div> <% else %> <%= markup %> <% end %>

Cool, OK thanks. I'll hold off applying this change as we might not need any of it if the margins will be handled automatically.

andysellick · 2024-10-21T08:48:48Z

app/views/landing_page/blocks/_govspeak.html.erb

Wondering if it would be easier to implement the shared helper for bottom margin on the govspeak component directly, then just pass the values required from here?

andysellick · 2024-10-21T08:53:34Z

app/views/landing_page/blocks/_govspeak.html.erb

@@ -1,3 +1,18 @@
+<%-


Nitpick: I don't think it's necessary to use <%- here (also I think the - is supposed to be in the closing marker) as this code shouldn't take up any line space.

andysellick · 2024-10-21T09:00:06Z

lib/data/landing_page_content_items/landing_page.yaml

@@ -85,6 +85,8 @@ blocks:
    content: <p>Right content!</p>
 - type: govspeak
  content: <h2>Statistics</h2>
+  margin_top: 5
+  margin_bottom: 5


I think we want to avoid having margins specified in the content item, because that implies that this is something that will be configured by the user, which I don't think we would want (could potentially break the page design).

Govspeak is a slightly tricky one, but maybe something that could be handled internally to the govspeak component. For the other blocks that need bottom margin, maybe the approach could be to remove their existing margins and rewrite your bottom margin class as a mixin, then just add it to their Sass?

Yes I did weigh this up but felt that given possible uncertainly over final content, it might be useful for content editors to have some control of how much spacing to apply around govspeak elements. As you note though, there's always the possibility of that control causing design issues. Would you prefer I remove this option?

For your other suggestion, do you mean avoid the use of the named class app-margin-bottom-responsive and instead use a mixin? This seems less flexible to me, but perhaps it's just apples and oranges.

To a degree, this gets us into the heart of the problem with this whole "blocks" approach.

If the user of the publishing system is allowed to pick from a list of arbitrary blocks, and stack and nest them in any order they like, it's going to be a significant challenge for the system to get the right margins and padding for every block in every context.

In particular, if there are examples like "if a video block follows a hero image, then the margin-bottom on the hero image should be X, but if a govspeak block follows a hero image then the margin-bottom on the hero image should be Y" then we're going to get into a really mucky place.

If it's possible for every block to have the same margins / padding as every other kind of block (at least at the top level), and for them to stack nicely in a way that doesn't ruin the designs, I think that's the ideal outcome.

If on the other hand, blocks need different margins in different contexts, I think it's probable more pragmatic to allow the user to provide their own margins (as we do here), perhaps while holding our noses a bit and telling ourselves "it's okay, someone from GDS will always review this content before it goes live".

If it's possible for every block to have the same margins / padding as every other kind of block (at least at the top level), and for them to stack nicely in a way that doesn't ruin the designs, I think that's the ideal outcome.

I'd personally prefer this outcome as it yields the simplest frontend solution, using something like the lobotomised owl selector:

* + * { margin-bottom: XX }

I know that's not in-keeping with our component/block based methodology overall, nevertheless it's a nice way to create a vertical rhythm using a standardised bottom margin. And it means the system can handle any combination/order of components/blocks, and content editors don't need to worry about how they will look.

Allowing content authors control over spacing could introduce inconsistencies as well as allowing the possibility that they could overlook problems, particularly if margins are different at different breakpoints. I'd vote for the consistent spacing approach.

Also, if we use a responsive margin in the selector above, then we can tweak the spacing for mobile/tablet/desktop while still maintaining consistency.

The exception would be govspeak content/headings, which probably would not want the same amount of consistent bottom margin as typically these are related to the following component.

How about this as a guiding principle?

All components/blocks get margin-bottom: XX when used as a direct child of the main content container, except govspeak components that are just text and relate to the following component. These should get margin-bottom: YY.

maxgds · 2024-10-21T09:33:17Z

app/assets/stylesheets/views/_landing_page.scss

+  }
+
+  @include govuk-media-query($from: desktop) {
+    margin-bottom: 80px;


Are you sure this figure is right? Didn't look like that much to me on the PDF.

Also, have you looked at using the responsive rather than the static spacing from the design system - ideally we would let that do the hard work rather than hand coding the break points ourselves.

I eyeballed it based on the Figma design, which doesn't seem to provide a way of measuring it exactly. I can find out the exact figure and change it as necessary.

I did look at the responsive scale, but it doesn't provide numbers that match the design. If we want to use the responsive spacing then we'd need the design to be changed to match it.

Worth a chat with Jeremy to get his thoughts on both getting an accurate figure and seeing if it can align with the DS scales, even if the answer is no.

matthillco · 2024-10-22T14:12:04Z

We've mostly solved this now in these other PRs:

Apply responsive bottom margin to blocks #4306
Use header block in demos #4308
Add shared helper and component wrapper helper to govspeak component #4325
Add documentation and test for govspeak margin #4327

I'll leave this PR open for now in case there's any further discussions to be had, but I suspect we'll be able to close this once we've had a design review of those PRs above.

matthillco · 2024-10-29T14:58:23Z

I'm closing this now as we've effectively solved the issues discussed.

Add a responsive bottom margin class for general use

c541786

govuk-ci temporarily deployed to govuk-frontend-app-pr-4296 October 18, 2024 15:12 Inactive

Add options to govspeak blocks for controlling vertical margins

ff254b8

matthillco force-pushed the landing-page-extra-margins branch from 5c857cd to ff254b8 Compare October 18, 2024 15:24

govuk-ci temporarily deployed to govuk-frontend-app-pr-4296 October 18, 2024 15:25 Inactive

andysellick reviewed Oct 21, 2024

View reviewed changes

maxgds reviewed Oct 21, 2024

View reviewed changes

matthillco closed this Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Options for adding extra vertical spacing between blocks #4296

Options for adding extra vertical spacing between blocks #4296

matthillco commented Oct 18, 2024 •

edited

Loading

andysellick left a comment

andysellick Oct 21, 2024

andysellick Oct 21, 2024

matthillco Oct 21, 2024 •

edited

Loading

maxgds Oct 21, 2024

matthillco Oct 21, 2024

andysellick Oct 21, 2024

matthillco Oct 21, 2024

andysellick Oct 21, 2024

andysellick Oct 21, 2024

matthillco Oct 21, 2024

andysellick Oct 21, 2024

matthillco Oct 21, 2024

andysellick Oct 21, 2024

andysellick Oct 21, 2024

andysellick Oct 21, 2024

matthillco Oct 21, 2024 •

edited

Loading

richardTowers Oct 21, 2024

matthillco Oct 21, 2024 •

edited

Loading

andysellick Oct 21, 2024

matthillco Oct 21, 2024

matthillco Oct 21, 2024 •

edited

Loading

maxgds Oct 21, 2024

matthillco Oct 21, 2024

maxgds Oct 21, 2024

matthillco commented Oct 22, 2024

matthillco commented Oct 29, 2024

Options for adding extra vertical spacing between blocks #4296

Options for adding extra vertical spacing between blocks #4296

Conversation

matthillco commented Oct 18, 2024 • edited Loading

What

Why

andysellick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthillco Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthillco Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthillco Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthillco Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthillco commented Oct 22, 2024

matthillco commented Oct 29, 2024

matthillco commented Oct 18, 2024 •

edited

Loading

matthillco Oct 21, 2024 •

edited

Loading

matthillco Oct 21, 2024 •

edited

Loading

matthillco Oct 21, 2024 •

edited

Loading

matthillco Oct 21, 2024 •

edited

Loading