search

django-crispy-forms / crispy-test-project

Simple Django project for testing https://github.com/maraujop/django-crispy-forms based on https://gist.github.com/maraujop/1838193
https://crispy-test-project.herokuapp.com
9 stars 12 forks source link

Added Accordion examples #23

Open cpina opened 3 years ago

smithdc1 commented 3 years ago

Hi @cpina

Thanks for all your effort on this topic. I don't have the headspace today to think about this all fully, but thought I could make some progress on this PR.

If I understand the issue correctly we're getting strange behavior when two groups are named the same. That is two groups open / close when you click one item.

This comment here suggests that we shouldn't allow two groups to be named the same and we should instead raise an error? If so, why would we merge an incorrect layout to master? I'm really happy to take a working layout here, as I think we're missing accordions at the moment

https://github.com/django-crispy-forms/django-crispy-forms/pull/1099/files#r544494764

cpina commented 3 years ago

Thanks for all your effort on this topic. I don't have the headspace today to think about this all fully, but thought I could make some progress on this PR.

I understand - it required some proper headscape for me to do it! But when got all the pieces in the head (Accordion, AccordionGroup, rendering steps) it's a logic solution.

If I understand the issue correctly we're getting strange behavior when two groups are named the same. That is two groups open / close when you click one item. This comment here suggests that we shouldn't allow two groups to be named the same and we should instead raise an error? If so, why would we merge an incorrect layout to master? I'm really happy to take a working layout here, as I think we're missing accordions at the moment

This pull request for crispy-test-project with the django-crispy-forms 1.10.0 does not work correctly. It needs https://github.com/django-crispy-forms/django-crispy-forms/pull/1099/ to work correctly.

The enforcing of the unique name happens (already happened, this is old code not related to these PRs) here: https://github.com/django-crispy-forms/django-crispy-forms/blob/master/crispy_forms/bootstrap.py#L351

The fix is in: https://github.com/django-crispy-forms/django-crispy-forms/pull/1099/commits/a12e9baf47a33702e78a982c9b82a4d9216ab30a : it assigns the data_parent correctly of the "child" (AccordionGroup).

What's not possible is to re-use the same instance of an AccordionGroup. This code is correct here: https://github.com/django-crispy-forms/crispy-test-project/pull/23/commits/2944b4569c954def318d72136914eb4536125832#diff-10851c229b1715f41afc8f7921dd9cc1d749f0ec89307e13c9ffd16fe142e1bcR188

But if I had done:

accordion_group_1 = AccordionGroup('Accordion Group 1', 'text_input_accordion1')
accordion_group_2 = AccordionGroup('Accordion Group 2', 'text_input_accordion2')

    helper = FormHelper()
    helper.layout = Layout(

        Accordion(accordion_group_1, accordion_group_2),
        HTML('<strong>Second accordion:</strong>'),
        Accordion('accordion_group1, accordion_group_2),)

above code doesn't work. When I was testing it I realised that django-crispy-forms already emits a warning because of rendering a field twice, in the re-used AccordionGroup.