nhsuk / nhsuk-frontend

NHS.UK frontend contains the code you need to start building user interfaces for NHS websites and services.
https://nhsuk.github.io/nhsuk-frontend/
MIT License
606 stars 106 forks source link

Top padding not applied to fieldset legends when they appear after a paragraph. #953

Open edwardhorsford opened 1 month ago

edwardhorsford commented 1 month ago

Bug Report

This section of SASS adds top padding to headings where they appear after a paragraph. It doesn't apply to fieldset-legends though - so if you're using a radio question after a paragraph, it looks too close.

What is the issue?

Fieldset legends should be included in the padding override.

frankieroberto commented 1 month ago

@edwardhorsford screenshots might help explain this?

edwardhorsford commented 1 month ago

Here's a screenshot from my service. There's several h2s and a legend. The h2s get extra top padding when they follow a paragraph. However the legend (which should visually look like an h2) does not - this causes it to be too close to the proceeding paragraph. Screenshot 2024-05-24 at 15 21 28

edwardhorsford commented 1 month ago

It's probably tricky to extend the current SASS to cover fieldset legends and labels because there's so many different combinations (and they're not direct siblings). I asked ChatGPT and it suggested a refactor like this:

@mixin nhsuk-heading-padding($large-elements, $small-elements) {
  @each $element in $large-elements {
    .nhsuk-body-l + #{$element},
    .nhsuk-body-m + #{$element},
    .nhsuk-body-s + #{$element},
    .nhsuk-list + #{$element},
    .nhsuk-body-l + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-m + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-s + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-list + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-l + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-body-m + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-body-s + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-list + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element} {
      @include nhsuk-responsive-padding(4, "top");
    }
  }

  @each $element in $small-elements {
    .nhsuk-body-l + #{$element},
    .nhsuk-body-m + #{$element},
    .nhsuk-body-s + #{$element},
    .nhsuk-list + #{$element},
    .nhsuk-body-l + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-m + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-s + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-list + .nhsuk-form-group > .nhsuk-label-wrapper > #{$element},
    .nhsuk-body-l + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-body-m + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-body-s + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element},
    .nhsuk-list + .nhsuk-form-group .nhsuk-fieldset > .nhsuk-fieldset__legend#{$element} {
      padding-top: nhsuk-spacing(1);

      @include mq($from: tablet) {
        padding-top: nhsuk-spacing(2);
      }
    }

    .nhsuk-lede-text + #{$element} {
      padding-top: 0;
    }
  }
}

@include nhsuk-heading-padding(
  ('.nhsuk-heading-l', '.nhsuk-label--l', '.nhsuk-fieldset__legend--l'),
  ('.nhsuk-heading-m', '.nhsuk-heading-s', '.nhsuk-label--m', '.nhsuk-fieldset__legend--m')
);

We probably wouldn't want exactly this, but the refactor might be clearer.

edwardhorsford commented 1 month ago

Have also raised on GOVUK-Frontend.