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

Fixed form-control classes with checkboxes #6

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

powderflask
Copy link
Contributor

handle case where checkbox is rendered without label.

regression test added: test fails for ver. 2022.1 passes with this fix.

@powderflask
Copy link
Contributor Author

Looks like tox envs may be out-of-date? I didn't touch any of that. I can try to fix the CI by removing deprecated versions and resolving dependency issues in tox envs if you like?

@smithdc1
Copy link
Member

smithdc1 commented Jan 7, 2024

Yes. It's been a while since I've updated the supported versions matrix. If you have the capacity to look at it, that would be amazing.

@powderflask
Copy link
Contributor Author

Happy to. I've updated the supported versions matrix, but django 5 introduced an innocuous change that breaks some of the tests: https://docs.djangoproject.com/en/5.0/releases/5.0/#forms

This change breaks 5 tests that (1) use SampleForm, which provides help_text for email field, and thus input acquires the new aria-describedby attribute; and (2) uses an html diff with a pre-rendered results file.

I can think of a couple complicated ways to fix this using conditionals based on Dj version (yuk), but hoped you might suggest a preferred method to resolve these 5 broken tests?

Everything else is working smoothly.

@powderflask
Copy link
Contributor Author

Actually, I see you are already bifurcating some tests like this based on Dj version. So I'll copy that pattern and make the PR, you can review the code then to verify it is consistent with rest of tests.

@powderflask
Copy link
Contributor Author

sigh... it gets worse.
Django assumes the help text element will have id="{{ field.auto_id }}_helptext" (see django/forms/field.html) whereas bootstrap3/help_text.html template has uses id="hint_{{ field.auto_id }}""

This means the new aria attribute rendered on the field itself refers to a non-existent id.
As per release notes, django then adds another attribute, aria-invalid="true". Django is also adding aria-invalid="true" to a few others fields too, not sure why.

Brute force fix required duplicating 7 pre-rendered test results and adding version-conditional logic to the 5 broken tests.
It's all working now, so I'll send a PR so you can review these changes. Just forewarning it's pretty hideous and a substantial increase in the number of pre-rendered test results that need to be maintained.

Let me know if you can think of an approach that will be cleaner, can do a little more work on this if needed.

@powderflask powderflask mentioned this pull request Jan 7, 2024
handle case where checkbox is rendered without label.

regression test added: test fails in 2022.1 passes with this fix.
@smithdc1
Copy link
Member

smithdc1 commented Jan 9, 2024

@powderflask -- thanks for all your contributions here! 🙏

@smithdc1 smithdc1 changed the title fix for issue #5 Fixed form-control classes with checkboxes Jan 9, 2024
@smithdc1 smithdc1 merged commit 1dbae65 into django-crispy-forms:main Jan 9, 2024
4 checks passed
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.

2 participants