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

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

Merged
merged 12 commits into from
Jul 22, 2024

Conversation

kosdmit
Copy link
Contributor

@kosdmit kosdmit commented Jul 1, 2024

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
Copy link
Member

smithdc1 commented Jul 7, 2024

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
Copy link
Contributor Author

kosdmit commented Jul 7, 2024

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?

@kosdmit kosdmit closed this Jul 7, 2024
@kosdmit kosdmit deleted the issue-168 branch July 7, 2024 08:37
@smithdc1
Copy link
Member

smithdc1 commented Jul 8, 2024

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
Copy link
Contributor Author

kosdmit commented Jul 13, 2024

Hi @smithdc1,

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

  • Reverted incorrect changes in test result's templates. They don't use 'active' class as per the bootstrap docs.
  • The tests that were failing by the django-crispy-form reason, get compatible with current lib versions parameter. It also marked as xfail with condition specified by crispy_forms version.

@kosdmit kosdmit reopened this Jul 13, 2024
tests/test_layout_objects.py Outdated Show resolved Hide resolved
@smithdc1
Copy link
Member

Could you add an entry to the changelog?

tox.ini Outdated Show resolved Hide resolved
@smithdc1
Copy link
Member

Thank you! 🥇

@smithdc1 smithdc1 merged commit 9a3e43f into django-crispy-forms:main Jul 22, 2024
6 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.

Accordion and AccordionGroup ignore css_class attribute.
3 participants