Add missing page heading classes

[matthew-shaw]

Some of the test fixture macros were missing the additional classes for labels and legends as page headings, where isPageHeading had been set to true. This PR adds them in to the macro options, along with the expected HTML output.

In terms of the tests themselves this should have no impact, they will pass with or without as the output matches the expected HTML. But for projects that have ported GOV.UK Frontend to other languages and templating engines such as GOV.UK Frontend Jinja and GOV.UK React JSX, these fixtures are also used for visual component demos as well, so the classes obviously affect the rendered output.

Examples of user facing demos where headings aren't rendered correctly due to missing classes in test fixtures:

• Character count
• Checkboxes
• File upload
• Input
• Radios
• Select
• Textarea

[vanitabarrett]

Thanks for raising this PR, @matthew-shaw . I think some context may be helpful here:

The fixtures that you’ve edited in this PR are automatically generated from YAML examples for each component. These examples live in a [component-name].yaml file in the /src directory of this repo, for example accordion.yaml

These yaml examples serve several purposes. They are used:

• in our review app, as visual examples of components in different states
• in our component tests, as examples for us to render and test that the correct attributes are being applied as expected
• to generate the fixtures which we ship in the govuk-frontend npm package, to allow maintainers of ports to test their HTML against ours

In this case, we want to check that isPageHeading isn’t coupled to how the component looks, i.e: we’ve made the intentional decision to include a visual example where isPageHeading is set, but no classes are passed in, to ensure 1) wrapping the element in a heading tag isn’t tied in with visual presentation, and 2) that no default styles might be influencing how the heading is presented.

Having said all that, we’re also aware that all of this isPageHeading logic is really something that belongs to the label and fieldset components, and it’s just something that gets passed through from all of our other input elements.

TLDR: We’d like to suggest the following:

• accept your change to add classes to the input element examples (this PR will need to be changed to edit the yaml files rather than the fixtures directly)
• at the same time, add more examples to the label and fieldset yaml files, so there are examples with no class and classes of different sizes

@matthew-shaw - would you like to make those changes? I’m equally happy to raise a PR myself, if that would be easier for you.

[matthew-shaw]

Thanks very much for taking a look at this @vanitabarrett really appreciate it.

Apologies for editing the packaged dist assets instead of the source, I didn't realise they were generated but makes complete sense now. Also interesting that your team use these for visual examples of components too, thanks for the links.

I'm happy to update this PR to change the source component yaml files rather than the output test fixtures. I'll also have a go at adding those examples/tests to the labels and fieldsets, but may need to ask you for advice on approach before review.

[matthew-shaw]

Hi @vanitabarrett I've pushed the changes you suggested, sorry about the commit history! 😳 (hopefully the PR can squash it)

• I've removed the changes to the generated fixture.json files and remade the changes in the source [component].yaml files.
• Also added new examples for labels and fieldsets as page headings in the xl, l, m and s sizes, along with isPageHeading set and no additional classes applied. That should cover the required test scenarios.

My only question is; should the examples without additional classes set also be hidden? Since there's no visual difference I'm not sure if you want them shown in your review app?

Other than that, ready for review again. Thanks! 😃

[vanitabarrett]

Thanks for making those changes @matthew-shaw ! Unfortunately this PR is proving a bit tricky to rebase given some other changes that have been merged into the main branch since, so I've figured it was easier to create a new PR here: #2659

Hopefully that's ok, and I'll close this PR now (but link back to it from the new PR so our conversations are still preserved)