search

django-crispy-forms / crispy-bootstrap5

Bootstrap5 template pack for django-crispy-forms
MIT License
451 stars 72 forks source link

Add modal template and tests #151

Closed pxwxnvermx closed 10 months ago

pxwxnvermx commented 1 year ago

@smithdc1 Hi, I have added the modal html and also the required tests as you pointed out in the other PR. Can you please review this?

smithdc1 commented 10 months ago

This looks good. I just want to have a think about aria-hidden="true" which appears in the example. It's set by the JS when you open/close it, but we're not adding it on page load.

https://getbootstrap.com/docs/5.2/components/modal/#live-demo

Likely the safest option would be to add it as per the example, if in doubt.

pxwxnvermx commented 10 months ago

This line of code from django-crispy-forms sets up the aria labelledby, without the correct aria labelledby attribute the tests are failing right now. Also removing the aria-hidden="true" will also lead to test failure as the result would defer from the parse_form return.

smithdc1 commented 10 months ago

https://bootstrapdocs.com/v3.0.3/docs/javascript/#modals

Bootstrap3 docs have some useful guidance on aria attributes. I think it's likely we can update crispy-forms core so these always get added across all template packs as it seems the same across bootstrap versions.

smithdc1 commented 10 months ago

I think it's likely we can update crispy-forms core so these always get added across all template packs as it seems the same across bootstrap versions.

So....

Previously bootstrap docs recommended it. But it got removed in https://github.com/twbs/bootstrap/issues/16020

Testing with NVDA and firefox it seems ok to me without it. It reads the button to launch the modal but doesn't read anything about the modal until it's visible. Reading that linked PR it seems to be due to .modal having display:none. I'm not an accessibility expert.

Therefore this PR, if we can fix the aria-labelledby I think we're good to get this merged.

pxwxnvermx commented 10 months ago

Hi @smithdc1 I have updated this PR to use the correct aria-labelledby for both the template and the test output. Is there anything else that might be needed on this PR?

smithdc1 commented 10 months ago

@pxwxnvermx I think this is good to go. I added a release note.

Thanks for the contribution!