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

Sets form to no-validate, so we can use Drupal validation instead #800

Closed
wants to merge 2 commits into from

Conversation

markconroy
Copy link
Member

Closes #503

What does this change?

HTML 5 validation is not very good. Drupal's form validation is much better.

This simply removes HTML 5 validation from forms - same approach as Disable HTML5 Validation module - so Drupal's form validation can take over instead.

Before:

Screenshot 2024-12-18 at 14 07 16

After:

Screenshot 2024-12-18 at 14 06 15

@markconroy markconroy requested a review from finnlewis December 18, 2024 14:14
@andybroomfield
Copy link
Contributor

Would we be able to put this behind a setting / config option? Be useful to introduce gradually to test.
Could be some issues if custom modules are dependant on html5 validation.

@andybroomfield
Copy link
Contributor

Also I think this might be better in localgov_core?

@andybroomfield
Copy link
Contributor

I tested and it works well, in that I get the Drupal validation errors and it makes a lot more sense. I'll need to check BHCC code to see if we are dependant on html5 validation anywhere.

@markconroy
Copy link
Member Author

I wonder would it make sense as a theme setting? Or do we want it in a module to disable HTML5 validation everywhere?

@finnlewis
Copy link
Member

I tested and it works well, in that I get the Drupal validation errors and it makes a lot more sense. I'll need to check BHCC code to see if we are dependant on html5 validation anywhere.

@andybroomfield [in the distant past, you commented](See: localgovdrupal/localgov_base#367 (comment)
) that:

Although this is focused on user login, and we should still fix the issue anyway for these cases, we should move away from using HTML5 validation if we want to match GDS guidelines.
see https://design-system.service.gov.uk/patterns/validation/

I see that the GDS guidelines specifically say "Turn off HTML5 validation."

So I agree with your previous comment and think we should merge this and roll out better validation by default.

What's the argument for making it optional?

If it really needs to be optional, we can use https://www.drupal.org/project/disable_html5_validation and install by default, allowing people to uninstall if desired.

Copy link
Member

@finnlewis finnlewis left a comment

Choose a reason for hiding this comment

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

I think we should go with this and happy to approve, but @andybroomfield I'd like you to be OK with this too.

@finnlewis
Copy link
Member

Sorry, only just spotted the PHP static analysis failure, so just added a commit to address that.

@finnlewis
Copy link
Member

All test pass now too :)

@finnlewis
Copy link
Member

@millnut Quick question: are hooks (like this one) still OK in the .profile file or do we want to move them to a module?

@finnlewis finnlewis requested a review from millnut January 15, 2025 17:58
@millnut
Copy link
Member

millnut commented Jan 16, 2025

I agree with @andybroomfield this should probably live in localgov_core so we don't have to move it at a later date with recipes.

@markconroy
Copy link
Member Author

I have created the same PR in localgov_core now so we can merge that one instead if we are in agreement.

localgovdrupal/localgov_core#267

@finnlewis
Copy link
Member

Thanks @markconroy

Closing this one in favour of localgovdrupal/localgov_core#267

@finnlewis finnlewis closed this Jan 21, 2025
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.

Disable HTML5 Validation and use Drupal's validation instead.
4 participants