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

Added Accordion examples #23

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cpina
Copy link
Member

@cpina cpina commented Dec 18, 2020

No description provided.

@smithdc1
Copy link
Member

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

cpina commented Jan 10, 2021

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 django-crispy-forms/django-crispy-forms#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: django-crispy-forms/django-crispy-forms@a12e9ba : 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: 2944b45#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.

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