django-crispy-forms / crispy-bootstrap5

Bootstrap5 template pack for django-crispy-forms
MIT License
462 stars 76 forks source link

Add missing css_class to accordion and accordion-group templates #180

Closed kosdmit closed 3 months ago

kosdmit commented 4 months ago

This PR is based on PR #169, and it completely resolves issue #168 .

I've fixed accordion.html and accordion-group.html templates and written a few tests for checking css_classes is rendered correctly. I've also fix some tests templates, because the first accordion item should have css_class 'active' by default (look comment in issue for details.

smithdc1 commented 4 months ago

Thanks for the PR!

Just one comment on about this:

because the first accordion item should have css_class 'active' by default

Is this the case with Bootstrap 5? I can't see any reference to this class in their docs and trying it locally seems to indicate there are no styles for that class?

kosdmit commented 4 months ago

You are right, that is my mistake, bootstrap 5 does not use 'active' class. But we have probably a legacy method in django-crispy-forms which adds 'active' to css_class string:

class Container(Div):
    def render(self, form, context, template_pack=TEMPLATE_PACK, **kwargs):
        if self.active:
            if "active" not in self.css_class:
                self.css_class += " active"
        else:
            self.css_class = self.css_class.replace("active", "")
        return super().render(form, context, template_pack)

I think it's would right to add a separate attr to define activity of item, and do not add this class to css_class string. What do you think?

smithdc1 commented 4 months ago

I was wondering where this was actually required, seems that it is for tabs rather than accordion.

Maybe we should move render() method you provided above to Tab from Container? I can't see active being on an accordion going back to Bootstrap 3, see docs but would need to do some further testing of that.

kosdmit commented 3 months ago

Hi @smithdc1,

I've done some work to close that issue in this template pack:

smithdc1 commented 3 months ago

Could you add an entry to the changelog?

smithdc1 commented 3 months ago

Thank you! 🥇