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

Fix Error Summary component outputting list HTML when no errorList is provided #4971

Merged
merged 1 commit into from
May 17, 2024

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented May 7, 2024

Modifies the Error Summary component and documentation to no longer require that the errorList parameter be set, and to not render associated HTML if it's empty.

This is intended to benefit situations where an error may not be associated with user input or any particular form field (e.g. a fault with the service), and where being able to provide descriptionText/descriptionHtml may suffice to explain the issue to a user.

Based on the conclusions reached in #3864. Closes #3864.

Changes

  • Adds a new condition in the Nunjucks template to not output the ul.govuk-error-summary__list element if the errorList parameter is empty.
  • Marks errorList as not being required in Nunjucks parameter documentation.
  • Adds a review app example of an Error Summary with a title and description, but no error list.
  • Adds tests to ensure that ul.govuk-error-summary__list is output if there are error list items, and not if there isn't.

Thoughts

I wasn't 100% sure on where this would live in the changelog. It's ultimately a very minor logic and documentation update that could be sold as both a new feature ("You can now use the Error Summary without providing a list of errors") and a simple bugfix ("The Error Summary no longer outputs a list if there isn't anything to put in the list").

I ended up listing it as a bugfix, as it's ultimately a very inconsequential change (people could already not pass any errorList items, it'd just return the 'wrong' HTML), and renamed this PR appropriately.

@querkmachine querkmachine requested a review from a team May 7, 2024 11:14
@querkmachine querkmachine self-assigned this May 7, 2024
Copy link

github-actions bot commented May 7, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 113.37 KiB
dist/govuk-frontend-development.min.js 42.34 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 88.16 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 82.83 KiB
packages/govuk-frontend/dist/govuk/all.mjs 981 B
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 113.36 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 42.33 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB
packages/govuk-frontend/dist/govuk/init.mjs 4.86 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 78.45 KiB 40.31 KiB
accordion.mjs 22.71 KiB 12.85 KiB
button.mjs 5.98 KiB 2.69 KiB
character-count.mjs 22.4 KiB 9.92 KiB
checkboxes.mjs 5.83 KiB 2.83 KiB
error-summary.mjs 7.89 KiB 3.46 KiB
exit-this-page.mjs 17.1 KiB 9.26 KiB
header.mjs 4.46 KiB 2.6 KiB
notification-banner.mjs 6.26 KiB 2.62 KiB
password-input.mjs 15.15 KiB 7.25 KiB
radios.mjs 4.83 KiB 2.38 KiB
skip-link.mjs 4.39 KiB 2.18 KiB
tabs.mjs 10.13 KiB 6.11 KiB

View stats and visualisations on the review app


Action run for bb5358d

Copy link

github-actions bot commented May 7, 2024

Rendered HTML changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/template-default.html b/packages/govuk-frontend/dist/govuk/components/error-summary/template-default.html
index fe082a877..4cbbbc2f8 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/template-default.html
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/template-default.html
@@ -4,14 +4,14 @@
       There is a problem
     </h2>
     <div class="govuk-error-summary__body">
-      <ul class="govuk-list govuk-error-summary__list">
-        <li>
-          <a href="#example-error-1">The date your passport was issued must be in the past</a>
-        </li>
-        <li>
-          <a href="#example-error-2">Enter a postcode, like AA1 1AA</a>
-        </li>
-      </ul>
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            <a href="#example-error-1">The date your passport was issued must be in the past</a>
+          </li>
+          <li>
+            <a href="#example-error-2">Enter a postcode, like AA1 1AA</a>
+          </li>
+        </ul>
     </div>
   </div>
 </div>
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/template-mixed-with-and-without-links.html b/packages/govuk-frontend/dist/govuk/components/error-summary/template-mixed-with-and-without-links.html
index 0584fd22f..8fa5e6f6b 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/template-mixed-with-and-without-links.html
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/template-mixed-with-and-without-links.html
@@ -4,14 +4,14 @@
       There is a problem
     </h2>
     <div class="govuk-error-summary__body">
-      <ul class="govuk-list govuk-error-summary__list">
-        <li>
-          Invalid username or password
-        </li>
-        <li>
-          <a href="#example-error-1">Agree to the terms of service to log in</a>
-        </li>
-      </ul>
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            Invalid username or password
+          </li>
+          <li>
+            <a href="#example-error-1">Agree to the terms of service to log in</a>
+          </li>
+        </ul>
     </div>
   </div>
 </div>
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-description-only.html b/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-description-only.html
new file mode 100644
index 000000000..7e3edec80
--- /dev/null
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-description-only.html
@@ -0,0 +1,12 @@
+<div class="govuk-error-summary" data-module="govuk-error-summary">
+  <div role="alert">
+    <h2 class="govuk-error-summary__title">
+      There is a problem
+    </h2>
+    <div class="govuk-error-summary__body">
+      <p>
+        The file couldn&#39;t be loaded. Try again later.
+      </p>
+    </div>
+  </div>
+</div>
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-everything.html b/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-everything.html
index 70939452a..41cae1ddc 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-everything.html
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/template-with-everything.html
@@ -7,14 +7,14 @@
       <p>
         Please fix the errors below.
       </p>
-      <ul class="govuk-list govuk-error-summary__list">
-        <li>
-          Invalid username or password
-        </li>
-        <li>
-          <a href="#example-error-1">Agree to the terms of service to log in</a>
-        </li>
-      </ul>
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            Invalid username or password
+          </li>
+          <li>
+            <a href="#example-error-1">Agree to the terms of service to log in</a>
+          </li>
+        </ul>
     </div>
   </div>
 </div>
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/template-without-links.html b/packages/govuk-frontend/dist/govuk/components/error-summary/template-without-links.html
index f06a86695..6864f7ebe 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/template-without-links.html
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/template-without-links.html
@@ -4,11 +4,11 @@
       There is a problem
     </h2>
     <div class="govuk-error-summary__body">
-      <ul class="govuk-list govuk-error-summary__list">
-        <li>
-          Invalid username or password
-        </li>
-      </ul>
+        <ul class="govuk-list govuk-error-summary__list">
+          <li>
+            Invalid username or password
+          </li>
+        </ul>
     </div>
   </div>
 </div>

Action run for bb5358d

Copy link

github-actions bot commented May 7, 2024

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/_index.scss b/packages/govuk-frontend/dist/govuk/components/error-summary/_index.scss
index a472c4c1d..0cb1cc2cc 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/_index.scss
@@ -24,14 +24,21 @@
 
   .govuk-error-summary__body {
     p {
-      margin-top: 0;
-      @include govuk-responsive-margin(4, "bottom");
+      margin-bottom: 0;
+    }
+
+    > * + * {
+      @include govuk-responsive-margin(4, "top");
     }
   }
 
   // Cross-component class - adjusts styling of list component
   .govuk-error-summary__list {
-    margin-top: 0;
+    margin-bottom: 0;
+  }
+
+  // Remove the bottom margin from the last list item
+  .govuk-error-summary__list li:last-child {
     margin-bottom: 0;
   }
 
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/fixtures.json b/packages/govuk-frontend/dist/govuk/components/error-summary/fixtures.json
index 264396967..556aa6aca 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/fixtures.json
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/fixtures.json
@@ -19,7 +19,7 @@
             "hidden": false,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          <a href=\"#example-error-1\">The date your passport was issued must be in the past</a>\n        </li>\n        <li>\n          <a href=\"#example-error-2\">Enter a postcode, like AA1 1AA</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            <a href=\"#example-error-1\">The date your passport was issued must be in the past</a>\n          </li>\n          <li>\n            <a href=\"#example-error-2\">Enter a postcode, like AA1 1AA</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "without links",
@@ -34,7 +34,7 @@
             "hidden": false,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "mixed with and without links",
@@ -53,7 +53,18 @@
             "hidden": false,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n        <li>\n          <a href=\"#example-error-1\">Agree to the terms of service to log in</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n          <li>\n            <a href=\"#example-error-1\">Agree to the terms of service to log in</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
+        },
+        {
+            "name": "with description only",
+            "options": {
+                "titleText": "There is a problem",
+                "descriptionText": "The file couldn't be loaded. Try again later."
+            },
+            "hidden": false,
+            "description": "",
+            "previewLayoutModifiers": [],
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        The file couldn&#39;t be loaded. Try again later.\n      </p>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "with everything",
@@ -73,7 +84,7 @@
             "hidden": false,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        Please fix the errors below.\n      </p>\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n        <li>\n          <a href=\"#example-error-1\">Agree to the terms of service to log in</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        Please fix the errors below.\n      </p>\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n          <li>\n            <a href=\"#example-error-1\">Agree to the terms of service to log in</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "html as titleText",
@@ -88,7 +99,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      Alert, &lt;em&gt;alert&lt;/em&gt;\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      Alert, &lt;em&gt;alert&lt;/em&gt;\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "title html",
@@ -103,7 +114,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      Alert, <em>alert</em>\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      Alert, <em>alert</em>\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "description",
@@ -119,7 +130,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        Lorem ipsum\n      </p>\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        Lorem ipsum\n      </p>\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "html as descriptionText",
@@ -135,7 +146,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        See errors below (&gt;)\n      </p>\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        See errors below (&gt;)\n      </p>\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "description html",
@@ -151,7 +162,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        See <span>errors</span> below\n      </p>\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <p>\n        See <span>errors</span> below\n      </p>\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "classes",
@@ -167,7 +178,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary extra-class one-more-class\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary extra-class one-more-class\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "attributes",
@@ -186,7 +197,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" first-attribute=\"foo\" second-attribute=\"bar\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Invalid username or password\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" first-attribute=\"foo\" second-attribute=\"bar\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Invalid username or password\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "error list with attributes",
@@ -206,7 +217,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          <a href=\"#item\" data-attribute=\"my-attribute\" data-attribute-2=\"my-attribute-2\">Error-1</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            <a href=\"#item\" data-attribute=\"my-attribute\" data-attribute-2=\"my-attribute-2\">Error-1</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "error list with html as text",
@@ -221,7 +232,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          Descriptive link to the &lt;b&gt;question&lt;/b&gt; with an error\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            Descriptive link to the &lt;b&gt;question&lt;/b&gt; with an error\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "error list with html",
@@ -236,7 +247,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          The date your passport was issued <b>must</b> be in the past\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            The date your passport was issued <b>must</b> be in the past\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "error list with html link",
@@ -252,7 +263,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          <a href=\"#error-1\">Descriptive link to the <b>question</b> with an error</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            <a href=\"#error-1\">Descriptive link to the <b>question</b> with an error</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "error list with html as text link",
@@ -268,7 +279,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n        <li>\n          <a href=\"#error-1\">Descriptive link to the &lt;b&gt;question&lt;/b&gt; with an error</a>\n        </li>\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n        <ul class=\"govuk-list govuk-error-summary__list\">\n          <li>\n            <a href=\"#error-1\">Descriptive link to the &lt;b&gt;question&lt;/b&gt; with an error</a>\n          </li>\n        </ul>\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "autofocus disabled",
@@ -279,7 +290,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-disable-auto-focus=\"true\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-disable-auto-focus=\"true\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n    </div>\n  </div>\n</div>"
         },
         {
             "name": "autofocus explicitly enabled",
@@ -290,7 +301,7 @@
             "hidden": true,
             "description": "",
             "previewLayoutModifiers": [],
-            "html": "<div class=\"govuk-error-summary\" data-disable-auto-focus=\"false\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n      <ul class=\"govuk-list govuk-error-summary__list\">\n      </ul>\n    </div>\n  </div>\n</div>"
+            "html": "<div class=\"govuk-error-summary\" data-disable-auto-focus=\"false\" data-module=\"govuk-error-summary\">\n  <div role=\"alert\">\n    <h2 class=\"govuk-error-summary__title\">\n      There is a problem\n    </h2>\n    <div class=\"govuk-error-summary__body\">\n    </div>\n  </div>\n</div>"
         }
     ]
 }
diff --git a/packages/govuk-frontend/dist/govuk/components/error-summary/macro-options.json b/packages/govuk-frontend/dist/govuk/components/error-summary/macro-options.json
index d2b266790..747968a36 100644
--- a/packages/govuk-frontend/dist/govuk/components/error-summary/macro-options.json
+++ b/packages/govuk-frontend/dist/govuk/components/error-summary/macro-options.json
@@ -32,8 +32,8 @@
     {
         "name": "errorList",
         "type": "array",
-        "required": true,
-        "description": "The list of errors to include in the error summary.",
+        "required": false,
+        "description": "A list of errors to include in the error summary.",
         "params": [
             {
                 "name": "href",

Action run for bb5358d

@querkmachine querkmachine changed the title Allow Error Summary component to not include an error list Fix Error Summary component outputting list HTML when no errorList is provided May 7, 2024
@querkmachine querkmachine force-pushed the error-summary-list-requirement branch from 48de999 to dad5d78 Compare May 7, 2024 11:23
@querkmachine querkmachine marked this pull request as ready for review May 7, 2024 11:26
@querkmachine querkmachine force-pushed the error-summary-list-requirement branch from dad5d78 to 94bad4e Compare May 8, 2024 16:03
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Thanks for looking into that one. The changes to the templates look neat and sounds fine to have the change as a fix. 🙌🏻

I think it might need a little CSS update as well. With only the description, there's some extra spacing from the <p> that wraps the description.

Screenshot of the description only example for the Error summary, showing extra space

Maybe something like the following instead of the rule for .govuk-error-summary__body p:

.govuk-error-summary__body {
  > * {
  	margin-bottom: 0;
  	margin-top: 0;
  }
  
  > * + * {
  	@include govuk-responsive-margin(4, "top");
  }
}

We might also want a little design look and/or some guidance regarding when to use a description or a list with a single item, as the output is visually the same (spacing aside) but the underlying semantics are different.

Screenshot of the without links example for the Error summary

@querkmachine querkmachine force-pushed the error-summary-list-requirement branch from 94bad4e to c07ff96 Compare May 16, 2024 15:50
Copy link

github-actions bot commented May 16, 2024

Stylesheets changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index de91626e8..de0a3a8db 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -3171,18 +3171,21 @@ screen and (forced-colors:active) {
 }
 
 .govuk-error-summary__body p {
-    margin-top: 0;
-    margin-bottom: 15px
+    margin-bottom: 0
+}
+
+.govuk-error-summary__body>*+* {
+    margin-top: 15px
 }
 
 @media (min-width:40.0625em) {
-    .govuk-error-summary__body p {
-        margin-bottom: 20px
+    .govuk-error-summary__body>*+* {
+        margin-top: 20px
     }
 }
 
-.govuk-error-summary__list {
-    margin-top: 0;
+.govuk-error-summary__list,
+.govuk-error-summary__list li:last-child {
     margin-bottom: 0
 }
 

Action run for bb5358d

@querkmachine querkmachine force-pushed the error-summary-list-requirement branch from c07ff96 to bb5358d Compare May 16, 2024 15:54
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4971 May 16, 2024 15:54 Inactive
@querkmachine
Copy link
Member Author

@romaricpascal I've updated the PR to use a variant of your suggestion, though I note that it didn't completely solve the issue of uneven negative space between description and list variants, as list items also have some uncollapsed margin-bottom applied to them! I added a rule to remove the margin from the last list item too.

In future it'd probably be a good idea to refactor this component a little to potentially use a vertical flexbox and gap instead.

@36degrees 36degrees requested a review from romaricpascal May 17, 2024 08:25
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Good spot on these 5px of blank space after that last <li> 🦅 All good to go for me 😊

@querkmachine querkmachine merged commit 97f1a3e into main May 17, 2024
48 checks passed
@querkmachine querkmachine deleted the error-summary-list-requirement branch May 17, 2024 09:28
@mia-allers-gds
Copy link

mia-allers-gds commented May 20, 2024

I'm late to this but I have some thoughts. Basically I think we should roll back to the previous version with the +5px bottom margin.

The last line item in a paragraph or a banner or a box or a button should, in my opinion, have a very slightly bigger bottom margin. The reason for this is that there are often (though not always) descenders in the type (a descender is a letter that descends below the baseline, for example a y or a g )

These descenders will then sometimes make the last line appear closer to whatever sits below it. Additionally, looking at this particular change, the underline also makes the space look much smaller. And finally, the margin at the top reached the highest point of the type, the capital letters. The bottom margin extends from the baseline. This will also give create more whitespace at the top, which needs to be balanced by increasing the bottom margin.

querkmachine added a commit that referenced this pull request Jun 10, 2024
Reverses a visual change introduced in #4971 where the negative space at the bottom of the Error Summary component was reduced to equalise the container padding on all sides.

Although this was originally approved of by a designer on the team, further discussion after the 5.4.0 release concluded that having additional negative space below the Error Summary's content was desirable, as it gave extra space for text descenders to occupy (if present).
querkmachine added a commit that referenced this pull request Jun 11, 2024
Reverses a visual change introduced in #4971 where the negative space at the bottom of the Error Summary component was reduced to equalise the container padding on all sides.

Although this was originally approved of by a designer on the team, further discussion after the 5.4.0 release concluded that having additional negative space below the Error Summary's content was desirable, as it gave extra space for text descenders to occupy (if present).
domoscargin pushed a commit that referenced this pull request Jun 19, 2024
Reverses a visual change introduced in #4971 where the negative space at the bottom of the Error Summary component was reduced to equalise the container padding on all sides.

Although this was originally approved of by a designer on the team, further discussion after the 5.4.0 release concluded that having additional negative space below the Error Summary's content was desirable, as it gave extra space for text descenders to occupy (if present).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should errorList be a required field in ErrorSummary component?
4 participants