[draft] Update to govuk frontend 5.7.1 #287

RogerHowellDfE · 2024-12-30T23:15:02Z

Intent

Currently this targets govuk-frontend v5.2.0 (released 21 Feb 2024).

This draft PR is work towards supporting govuk-frontend v5.7.1 (released 11 Oct 2024).

Out of scope of this PR are new tag helpers such as the password component (new in v5.3.0 #286 ). Ditto for older components such as the header (#18 ) or footer (#17 ) tag helpers.

Changes

Remove redundant role attributes, per changes in govuk-frontend v5.3.0
Update HTML structure of character count component, per changes in govuk-frontend v5.3.0
Update test to check for presence of svg crest logo (previously png, per changes in v5.7.0

These changes are also flagged as changes per test suites, but seemingly no explicit mention in the govuk-frontend changelog.

Update error summary to not output an empty <ul>, per conformance test expectations
Handle autocapitalize on text input components, per conformance test expectations
TBC: conformance test failure on the with customised input wrapper case

Note: I did begin to do an incremental upgrade but all the headaches seem to be introduced in v5.3.0 (character count component) which makes this idea not so useful 🤷 .

…utput raw HTML to disk for easier diffing (e.g. in IDE)

- banner role on header (no usages found, no change) - contentinfo role on footer (no usages found, no change) - main role on main (done, see "_Layout.cshtml") - navigation role on nav (done, see component "Pagination") - complementary role on aside (no usages found, no change) - region role on section (no usages found, no change) - article role on article (no usages found, no change) See also: alphagov/govuk-frontend#4854

….7.0 alphagov/govuk-frontend#5376

…per govuk-frontend v5.4.0 alphagov/govuk-frontend#4971

…ontend v5.3.0 (fixing tag helper etc. is to follow) alphagov/govuk-frontend#4566

RogerHowellDfE · 2024-12-30T23:43:54Z

There are several minor changes in more recent versions, but the character component changes (introduced in v5.3.0) are proving to be a headache.

Specifically, this seems to be the first case where the govuk-form-group and the top-most / outermost element is shared / used as both the container and the "named" element.

This, unfortunately, then seems to break many of the assumptions/logic in the helper methods/functionality which assumes they are at separate levels of nesting

e.g., methods on FormGroupTagHelperBase don't necessarily have the *Contexts available
- the *Context is where data such as the max counts etc. are stored, and
- where *Context is provided, it doesn't seem to have access to the parent element
e.g., test helpers such as GenerateFormGroup don't seem to allow easy editing of the root element?).

Being mostly new to the internals, there is a decent chance I am missing something obvious however! 🥲

Similarly, I might be hamstringing myself by not wanting to make too substantial a change to method signatures etc.
... feel free to brutally wave away the proposed changes and signpost to a better approach!

RogerHowellDfE

Lots of comments flagging thoughts / TODOs etc.

Apologies it's not all fully finished and polished!

RogerHowellDfE · 2024-12-30T23:47:54Z

Directory.Build.props

+<!--    <GovUkFrontendVersion>5.2.0</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.3.0</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.3.1</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.4.0</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.4.1</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.5.0</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.6.0</GovUkFrontendVersion>-->
+<!--    <GovUkFrontendVersion>5.7.0</GovUkFrontendVersion>-->
+    <GovUkFrontendVersion>5.7.1</GovUkFrontendVersion>

-    <_NpmLibDirectory>$([MSBuild]::NormalizeDirectory('$(RepoRoot)', 'lib'))</_NpmLibDirectory>
+    <_NpmLibDirectory>$([MSBuild]::NormalizeDirectory('$(RepoRoot)', 'lib', 'govuk-frontend-$(GovUkFrontendVersion)'))</_NpmLibDirectory>


I was doing a lot of jumping around versions to try and figure out when things broke and do incremental upgrades. This led to lots of delays each time the version changed (re-downloading it etc.)

This change caches the govuk-frontend in versioned directories:

I'm explicitly flagging this as something I'm making use of currently on this branch, but it might not be desirable to merge in.

I like this. We could probably do away with the version.txt file with this change too.

RogerHowellDfE · 2024-12-30T23:49:50Z

src/GovUk.Frontend.AspNetCore/HtmlGeneration/ComponentGenerator.CharacterCount.cs

-        tagBuilder.InnerHtml.AppendHtml(formGroup);
-        tagBuilder.InnerHtml.AppendHtml(GenerateHint());


I have separated responsibilities here, with GenerateCharacterCount responsible for the outermost / root element only, and the content (label, hint, error, textarea, remaining count message) being passed in/injected as an argument.

RogerHowellDfE · 2024-12-30T23:50:39Z

src/GovUk.Frontend.AspNetCore/HtmlGeneration/ComponentGenerator.DateInput.cs

                type: "text",
-                item.Value,
+                value: item.Value,
                describedBy: null,
-                item.Autocomplete,
-                item.Pattern,
-                item.InputMode,
+                autocomplete: item.Autocomplete,
+                autocapitalize: null,
+                pattern: item.Pattern,
+                inputMode: item.InputMode,
                spellcheck: null,
-                disabled,
-                item.Attributes,
+                disabled: disabled,
+                attributes: item.Attributes,
                prefixContent: null,


Spurious IDE changes while experimenting.

TODO: Remove these.

RogerHowellDfE · 2024-12-30T23:57:18Z

src/GovUk.Frontend.AspNetCore/TagHelpers/TextInputTagHelper.cs

+    /// <summary>
+    /// The <c>autocapitalize</c> attribute for the generated <c>input</c> element.
+    /// </summary>
+    [HtmlAttributeName(AutocapitalizeAttributeName)]
+    public string? Autocapitalize { get; set; }
+


I've not done enough sleuthing to figure out when it got introduced, but autocapitalize is included in the upstream tests and results in a conformance test failure:

Xunit.Sdk.XunitException --- Actual: --- <div class="govuk-form-group"> <label class="govuk-label" for="input-with-autocapitalize-off"> Autocapitalize is turned off </label> <input class="govuk-input" id="input-with-autocapitalize-off" name="autocapitalize" type="text"> </input> </div> --- Exected: --- <div class="govuk-form-group"> <label class="govuk-label" for="input-with-autocapitalize-off"> Autocapitalize is turned off </label> <input class="govuk-input" id="input-with-autocapitalize-off" name="autocapitalize" type="text" autocapitalize="none"> </div> --- AssertEx.HtmlEqual() Failure * Missing Attribute Path: div(0) > input(1)[autocapitalize] Name: autocapitalize Value: none at GovUk.Frontend.AspNetCore.TestCommon.AssertEx.HtmlEqual(String expected, String actual, Predicate`1 excludeDiff, Boolean outputFullMarkupOnFailure) in C:\dev\govuk-frontend-aspnetcore\tests\GovUk.FrontEnd.AspNetCore.TestCommon\AssertEx.cs:line 81 at GovUk.Frontend.AspNetCore.ConformanceTests.ComponentTests.CheckComponentHtmlMatchesExpectedHtml[TOptions](ComponentTestCaseData`1 testCaseData, Func`3 generateComponent, Predicate`1 excludeDiff) in C:\dev\govuk-frontend-aspnetcore\tests\GovUk.Frontend.AspNetCore.ConformanceTests\ComponentTests.cs:line 49 at GovUk.Frontend.AspNetCore.ConformanceTests.ComponentTests.TextInput(ComponentTestCaseData`1 data) in C:\dev\govuk-frontend-aspnetcore\tests\GovUk.Frontend.AspNetCore.ConformanceTests\ComponentTests.TextInput.cs:line 17

RogerHowellDfE · 2024-12-31T00:04:20Z

tests/GovUk.FrontEnd.AspNetCore.TestCommon/AssertEx.cs

+            throw new XunitException("---\nActual: \n---\n" + actual.Replace(">", ">\n").Replace("<", "\n<").Replace(">\n\n<", ">\n<").Trim() + "\n\n" +
+                                     "---\nExected: \n---\n" + expected + "\n\n" +
+                                     "---\n" +
+                                     "" + sb.ToString());


The HTML diff is very useful/required to flag where the differences are, but it only gets so far if not able to visualise the broader context i.e. the syntax tree / html string it is referring to (a-la "stop at the third red house on the left").

I have found it useful to be able to quickly reference expected/actual HTML, to then try and figure out whether it is the container versus a nested element (or both) whose behaviour is wrong.

It's a bit of a hacky solution and might not be desirable to merge this, however?

RogerHowellDfE · 2024-12-31T00:26:37Z

src/GovUk.Frontend.AspNetCore/TagHelpers/CharacterCountTagHelper.cs

+        IHtmlContent GenerateCountRemainingMessage()
+        {
+            // TODO / FIXME: temporary hardcoded, need to pass through from somewhere? Unable to access context from here?
+            string textAreaDescriptionText = null;
+            var hasNoLimit = MaxLength is null && MaxWords is null; // TODO: This logic is reused in many places -- centralise it?
+
+            var hintId = $"{ResolveIdPrefix()}-info";
+
+            var content = hasNoLimit ? "" :
+                (textAreaDescriptionText ?? $"You can enter up to %{{count}} {(MaxWords.HasValue ? "words" : "characters")}")
+                .Replace("%{count}", (MaxWords.HasValue ? MaxWords : MaxLength).ToString());
+
+            var hintContent = new HtmlString(HtmlEncoder.Default.Encode(content));
+
+            var attributes = CountMessageAttributes.ToAttributeDictionary();
+            attributes.MergeCssClass("govuk-character-count__message");
+
+            return Generator.GenerateHint(hintId, hintContent, attributes);
+        }


Flagging the to-do here. It's one of the examples where there seems to be a mismatch between data being stored on the *Context but isn't available on the FormGroupTagHelperBase#GenerateFormGroupContent method / overrides.

Note that tests aren't failing with this dodgy implementation, but maybe this means additional tests are needed?

RogerHowellDfE · 2024-12-31T00:27:23Z

src/GovUk.Frontend.AspNetCore/TagHelpers/CharacterCountTagHelper.cs

+        {
+            // TODO / FIXME: temporary hardcoded, need to pass through from somewhere? Unable to access context from here?
+            string textAreaDescriptionText = null;
+            var hasNoLimit = MaxLength is null && MaxWords is null; // TODO: This logic is reused in many places -- centralise it?


Assuming the *Context could be made available, is it appropriate to centralise the hasNoLimit logic to it? Somewhere else, perhaps?

RogerHowellDfE · 2024-12-31T00:28:54Z

src/GovUk.Frontend.AspNetCore/Views/Shared/_GovUkPageTemplate.cshtml

    <div class="govuk-width-container @ViewBag.ContainerClasses">
        @RenderSection("BeforeContent", required: false)
-        <main class="govuk-main-wrapper @ViewBag.MainClasses" id="main-content" role="main" lang="@ViewBag.MainLang">
+        <main class="govuk-main-wrapper @ViewBag.MainClasses" id="main-content" lang="@ViewBag.MainLang">


v5.3.0

alphagov/govuk-frontend#4854

RogerHowellDfE · 2024-12-31T00:29:06Z

tests/GovUk.Frontend.AspNetCore.Tests/TagHelpers/PaginationTagHelperTests.cs

        // Assert
        var expectedHtml = @"
-<nav aria-label=""Pagination"" class=""govuk-pagination--block govuk-pagination"" role=""navigation"">
+<nav aria-label=""Pagination"" class=""govuk-pagination--block govuk-pagination"">


v5.3.0

alphagov/govuk-frontend#4854

RogerHowellDfE · 2024-12-31T00:29:19Z

tests/GovUk.Frontend.AspNetCore.Tests/TagHelpers/PaginationTagHelperTests.cs

        var actual = output.ToHtmlString();
        var expectedHtml = @"
-<nav aria-label=""search"" class=""govuk-pagination"" role=""navigation"">
+<nav aria-label=""search"" class=""govuk-pagination"">


v5.3.0

alphagov/govuk-frontend#4854

RogerHowellDfE · 2024-12-31T00:42:58Z

Hmm...

At the time of creating the PR I had only two failing tests (one for an i8n data attribute on the character count component, and one on the text input component in the with customised input wrapper scenario).

I clearly must have had something cached funny on my machine and doing a bunch of branch switching just now has unsettled things.

It seems like there are quite a few additional conformance tests failing due to boolean conversions. It might just be an intermittent thing on my end (workflow is pending approval so can't yet tell from the PR checks) but I'll explore those tomorrow/another day.

gunndabad · 2024-12-31T09:44:12Z

the character component changes (introduced in v5.3.0) are proving to be a headache.

That's what's stopped me so far from making this upgrade. There's a sizeable refactor of this library going on (see the dev branch); I might bring over some of its parts into main just to make this change at least do-able.

gunndabad · 2024-12-31T09:45:11Z

Thanks for PR @RogerHowellDfE. I think it would be good to upgrade to 5.3.0 first then upgrade to each frontend version separately, that way I can create a release for each version along the way.

RogerHowellDfE · 2024-12-31T11:32:27Z

the character component changes (introduced in v5.3.0) are proving to be a headache.

That's what's stopped me so far from making this upgrade.

Reassuring to hear it's not a "just me" issue having difficulty with the implementation! 😓 😁

There's a sizeable refactor of this library going on (see the dev branch); I might bring over some of its parts into main just to make this change at least do-able.

Apologies I missed the refactor happening on dev!

Is this something you might reasonably want some help with? I appreciate Some changes just need time to work through and adding extra people complicates things, so I promise to not be offended if you say no / not yet!

Thanks for PR @RogerHowellDfE. I think it would be good to upgrade to 5.3.0 first then upgrade to each frontend version separately, that way I can create a release for each version along the way.

I'm happy to close this PR in favour of this plan. I've had my fun getting to know the library a bit better, so I'll not shed a tear at its loss.

The changes are either trivial (no need to keep this PR for them), or relate to the character count component (not applicable once the refactor comes in?).

Apologies for the unsolicited noise without discussing first! 😄

gunndabad · 2024-12-31T11:36:15Z

@RogerHowellDfE I'm grateful for the help and there are definitely some bits from this PR I'd like to steal :-)

With hindsight having a completely separate branch that's been around for so long wasn't the right thing to do; I'm working on consolidating things now then changes should be easier to add on. I think an iterative approach to the changes I have planned is probably a better way than the big bang I was attempting previously.

RogerHowellDfE added 7 commits December 30, 2024 14:44

Make switching between govuk-frontend versions easier in tests, and o…

d5ae0c4

…utput raw HTML to disk for easier diffing (e.g. in IDE)

Switch test for crest presence from png to svg, per govuk-frontend v5…

07f9904

….7.0 alphagov/govuk-frontend#5376

Do not output an error summary <ul> element if no errors provided, …

e405dd5

…per govuk-frontend v5.4.0 alphagov/govuk-frontend#4971

Handle Autocapitalize test failure

cd34d66

Update CharacterCountTagHelperTests.cs to reflect changes in govuk-fr…

98971c3

…ontend v5.3.0 (fixing tag helper etc. is to follow) alphagov/govuk-frontend#4566

WIP -- CharacterCount

01abe7c

RogerHowellDfE commented Dec 31, 2024

View reviewed changes

RogerHowellDfE added 3 commits December 31, 2024 00:45

Update govuk-frontend version in the readme

447f522

dotnet format

8b97a43

Fix CS8600 warning/error.

67b4324

RogerHowellDfE closed this Dec 31, 2024

		tagBuilder.InnerHtml.AppendHtml(formGroup);
		tagBuilder.InnerHtml.AppendHtml(GenerateHint());

[draft] Update to govuk frontend 5.7.1 #287

[draft] Update to govuk frontend 5.7.1 #287

Uh oh!

Conversation

RogerHowellDfE commented Dec 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intent

Changes

Uh oh!

RogerHowellDfE commented Dec 30, 2024

Uh oh!

RogerHowellDfE left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerHowellDfE commented Dec 31, 2024

Uh oh!

gunndabad commented Dec 31, 2024

Uh oh!

gunndabad commented Dec 31, 2024

Uh oh!

RogerHowellDfE commented Dec 31, 2024

Uh oh!

gunndabad commented Dec 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RogerHowellDfE commented Dec 30, 2024 •

edited

Loading