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

Various fixes for html-validate (nested forms etc) #3115

Merged
merged 10 commits into from
Sep 4, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Sep 1, 2023

PR to fix various issues flagged by html-validate which we've temporarily ignored

Issues now fixed

To fix "forms within forms" I've split our example layout in two:

  1. Use layout-example.njk for examples without forms
  2. Use layout-example-form.njk for examples with forms

Issues already fixed

Plus some issues had already been fixed so we can remove their rules:

I'll open a separate PR for cookie banner specific things

We already have a wrapping `<form>` in `layout-example.njk`
Examples that don’t need a `<form>` wrapper can stay on the previous layout

1. Prevents nested `<form>` elements to fix #2609
2. Prevents unnecessary `<form>` element wrapping
@colinrotherham colinrotherham added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) assurance labels Sep 1, 2023
@netlify
Copy link

netlify bot commented Sep 1, 2023

You can preview this change here:

Name Link
🔨 Latest commit a448bd6
🔍 Latest deploy log https://app.netlify.com/sites/govuk-design-system-preview/deploys/64f20db50e503f0008c27f74
😎 Deploy Preview https://deploy-preview-3115--govuk-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Generally looks good but I don't quite understand the changes or the commit message for d540a4ealphagov/govuk-frontend#2830 is part of v5 so that fix hasn't gone out yet?

Buttons are all fixed but still got a few `<fieldset hidden="">` remaining
@colinrotherham
Copy link
Contributor Author

Generally looks good but I don't quite understand the changes or the commit message for d540a4e#2830 is part of v5 so that fix hasn't gone out yet?

@36degrees Ahh in that case, maybe we fixed them all here too?

We have failures for <fieldset hidden=""> but can't find any <button disabled="disabled"> anymore

Comes from where we pass an empty string into the attribute value:

attributes: {
id: "analytics",
hidden: ""
}

Remember this?

I'll update the example again

@36degrees
Copy link
Contributor

Realised we don't have any examples of disabled buttons so I suspect the rule was always disabled because of the hidden attribute on fieldsets? 🤔 Anyway, the comment change still makes sense.

@colinrotherham
Copy link
Contributor Author

Realised we don't have any examples of disabled buttons so I suspect the rule was always disabled because of the hidden attribute on fieldsets? 🤔 Anyway, the comment change still makes sense.

@36degrees Great, thanks

Yeah. Possibly just copied over from alphagov/govuk-frontend@500a02f

@colinrotherham colinrotherham merged commit 990a275 into main Sep 4, 2023
@colinrotherham colinrotherham deleted the html-validate-fixes branch September 4, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance 🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
Development

Successfully merging this pull request may close these issues.

2 participants