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

All sentences should be inside <p> (or the relevant semantic elements) #1611

Closed
colinrotherham opened this issue Oct 11, 2019 · 5 comments
Closed
Labels
breaking change feature request User requests a new feature

Comments

@colinrotherham
Copy link
Contributor

I'm going to add a few points that came out of a recent developer review, before three of our services are sent for an external accessibility audit.

Inset text and Panel components

We're currently using the govukInsetText() and govukPanel() macros and we've been flagged during a developer review for putting text inside <div> tags.

All three services were caught out by this.

The non-semantic HTML is the recommended usage on the GOV.UK Design System examples shown here:

https://design-system.service.gov.uk/components/inset-text/
https://design-system.service.gov.uk/components/panel/

You can see in these screenshots:

Inset text
Panel

Since other service teams may copy from the examples and make the same mistakes as us, I'd prefer it if the examples included semantic HTML by default! 😊

Suggested change:

Before

{{ params.html | safe if params.html else params.text }}

After

{% if params.html %}{{ params.html | safe }}{% else %}<p>{{ params.text }}</p>{% endif %}

But other macros might be affected too.

It's been raised before in alphagov/govuk-design-system#822

Thanks

@NickColley NickColley added the awaiting triage Needs triaging by team label Oct 11, 2019
@NickColley
Copy link
Contributor

Pre-triage comment: I believe that there are other components we'd want to review as part of considering this.

@colinrotherham
Copy link
Contributor Author

Sorry just realised we're flagged on one service only with warning text.

I'd expect a wrapping paragraph tag here too.

Warning text

Thanks @NickColley.

@hannalaakso
Copy link
Member

@colinrotherham We'd be interested in hearing whether this gets flagged in your external accessibility audit.

Have added this to 4.0 milestone as this would be a breaking change.

@kellylee-gds kellylee-gds added the feature request User requests a new feature label Nov 8, 2019
@adamliptrot-oc
Copy link

WHATWG deem the<p> element as only one way of markup up a paragraph. Semantically it seems not to have any impact. There could be styling implications of course.
https://html.spec.whatwg.org/multipage/dom.html#paragraph

@NickColley
Copy link
Contributor

Thanks @adamliptrot-oc, that's good context, I've had a chat about this with some people on the team...

We're going to close this for now as we want to avoid breaking changes unless it's necessary, but we're happy to revisit this again if we become aware of any real-world issues caused by the lack of <p> tags.

@NickColley NickColley removed this from the v4.0.0 milestone Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature request User requests a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants