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 error messages to use paragraph tags instead of spans #2452

Merged
merged 3 commits into from
Dec 3, 2021

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Dec 1, 2021

Part of #2083

Updates the error messages parent element to use paragraph tags instead of spans, as it's more semantically correct.

Read #2083 (comment) for full details of accessibility testing. There should be no visual difference following this change.

Closes #2083

Updates the error messages parent element to use paragraph tags instead
of spans, as it's more semantically correct.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2452 December 1, 2021 15:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2452 December 1, 2021 16:15 Inactive
@vanitabarrett vanitabarrett requested a review from a team December 1, 2021 16:17
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@vanitabarrett vanitabarrett changed the title Update error messages from span to paragraph Update error messages to use paragraph tags instead of spans Dec 2, 2021
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2452 December 2, 2021 11:47 Inactive
lfdebrux
lfdebrux previously approved these changes Dec 3, 2021
@lfdebrux lfdebrux dismissed their stale review December 3, 2021 12:07

I think I can see a visual change

@36degrees
Copy link
Contributor

I think I can see a visual change

Yep, good catch. I think there's an additional top margin coming from the user agent default styles for paragraphs?

Switching the error message to a paragraph means that it gets
any default browser styles for paragraphs applied. Looking at
https://browserdefaultstyles.com/#p this seems to be margin-top and
margin-bottom.

We already set margin-bottom on the error message. This commit
makes sure we set `margin-top` to `0` to reset any browser styles.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2452 December 3, 2021 12:16 Inactive
@lfdebrux
Copy link
Member

lfdebrux commented Dec 3, 2021

I've done a visual inspection in:

  • macOS / Safari
  • macOS / Chrome
  • macOS / Firefox with forced colours
  • Android / Samsung Internet (BrowserStack)
  • Windows / IE8 (BrowserStack)
  • Windows / IE11 with High Contrast Mode

Copy link
Member

@lfdebrux lfdebrux left a comment

Choose a reason for hiding this comment

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

Looks good to me

@vanitabarrett
Copy link
Contributor Author

As a note, @lfdebrux has also spotted that this slightly changes how this component looks when styles are disabled. As it's now a paragraph, the error message renders on its own line, but we think this is an improvement.

Before

screenshot_2021-12-03_at_13 10 29

After

screenshot_2021-12-03_at_13 10 52

@vanitabarrett vanitabarrett merged commit 0d71656 into main Dec 3, 2021
@vanitabarrett vanitabarrett deleted the error-messages-paragraphs branch December 3, 2021 13:17
@vanitabarrett vanitabarrett mentioned this pull request Dec 15, 2021
peteryates added a commit to x-govuk/govuk-form-builder that referenced this pull request Dec 18, 2021
Previously they were in spans but in version 4.0.0 of the design system
p is preferred because it's more semantically correct

alphagov/govuk-frontend#2452
andymantell added a commit to surevine/govuk-react-jsx that referenced this pull request Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants