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

Update fieldset examples to use macros #179

Closed
wants to merge 3 commits into from

Conversation

dashouse
Copy link

Currently all examples using the fieldset (when not using a component that has it integrated) use HTML only.

This updates all examples to use macros (thanks for teaching me how to do this @alex-ju)

@govuk-design-system-ci
Copy link
Collaborator

You can preview this change here:

Built with commit 9cdfd66

https://deploy-preview-179--govuk-design-system-preview.netlify.com

name: "passport-number"
}) }}
{% call govukFieldset({
"legendHtml": '<h3 class="govuk-heading-m govuk-!-mb-r1">Passport number</h3>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead here set a class for the legend, do we need the nested heading?

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a heading in a legend helps AT users to navigate using headings. (see #724), so I see it bringing value and no harm.

Copy link
Contributor

@36degrees 36degrees Feb 20, 2018

Choose a reason for hiding this comment

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

I think adding a class to the legend would also not work if the question needed a hint (unless the hint styles are defensive enough to be able to be used as a descendant of govuk-heading-m).


{{ govukInput({
"label": {
"html": 'Town or city'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use html here?


{{ govukInput({
"label": {
"html": '<span class="govuk-h-visually-hidden">Building and street line 2 of 2</span>'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we could pass through classes to label here and avoid using html?

@hannalaakso
Copy link
Member

hannalaakso commented Feb 20, 2018

Instead of placing h1 inside the label (which is not semantically correct), we could place the label inside the h1 which is correct. I know that this makes the API a bit more complex but it would:

  1. Achieve semantic headings
  2. Avoid wrapping single inputs in a fieldset
  3. Avoid having content read out twice to screen readers (no visual hiding of labels)

(Thanks for talking me through this @dashouse!)

@dashouse dashouse closed this Mar 1, 2018
@dashouse dashouse deleted the update-fieldset-examples-to-use-macros branch March 1, 2018 11:03
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.

None yet

6 participants