alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.16k stars 320 forks source link

Panel component - Template / Macro #558

Closed dashouse closed 6 years ago

dashouse commented 6 years ago

Title

titleText adds content inside an h2 with the class of govuk-c-panel__title

titleHtml also adds content inside theh2, meaning you cannot actually add HTML or add a class to this h2. If you try you end up with an empty h2 above or around your new HTML.

Empty div

The text argument is not conditional, therefore if a user wants to add only a title to their panel (it happens) they will end up with an empty div govuk-c-panel__body.

Current code

<div class="govuk-c-panel govuk-c-panel--confirmation
  {%- if params.classes %} {{ params.classes }}{% endif %}"
  {%- for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
  <h2 class="govuk-c-panel__title">
    {{ params.titleHtml | safe if params.titleHtml else params.titleText }}
  </h2>
  <div class="govuk-c-panel__body">
    {{ params.html | safe if params.html else params.text }}
  </div>
</div>

Suggested change (along the lines of)

<div class="govuk-c-panel
  {%- if params.classes %} {{ params.classes }}{% endif %}"
  {%- for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
  <h2 class="govuk-c-panel__title">
    {{ params.titleHtml | safe if params.titleHtml else params.titleText }}
  </h2>
  {% if params.html | safe if params.html else params.text %}
    <div class="govuk-c-panel__body">
      {{ params.html | safe if params.html else params.text }}
    </div>
  {% endif %}
</div>

I wonder if there is a broader review of components to check for other situations where empty divs are created when a user removes a component argument?

edwardhorsford commented 6 years ago

Possibly related - consider printing warnings where required arguments aren't provided (though it sounds like that isn't the case here).

kr8n3r commented 6 years ago

oh i see. yes, if neither titleHtml or titleText it will just be an empty h2 don't know whether we should enable adding just a class on h2, but we could do something like

{% if params.titleText %}
<h2 class="govuk-c-panel__title">
    {{ params.titleText }}
  </h2>
{% elif params.titleHtml %}
    {{ params.titleHtml | safe }} //make your own heading and content
{% endif %}