Skip to content

Add support for accordion-flush and accordion always open#63

Merged
smithdc1 merged 1 commit into
django-crispy-forms:mainfrom
JimiRecard:accordion-flush
Jun 18, 2021
Merged

Add support for accordion-flush and accordion always open#63
smithdc1 merged 1 commit into
django-crispy-forms:mainfrom
JimiRecard:accordion-flush

Conversation

@JimiRecard

Copy link
Copy Markdown
Contributor

This addresses #60, #61 and #62. Tests should work now.

The term "always open" sounds misleading to me, but it's what the folks at Bootstrap use. We could change this to something more descriptive if judged necessary.

I'm not sure about the way I pulled classes from bootstrap.py. It feels awkward. I was inheriting from the original classes before and adding just what I needed, but it somehow felt worse. Any suggestions?

@JimiRecard

Copy link
Copy Markdown
Contributor Author

And as I said that, most of the tests failed.

I don't know what went wrong. The only test that failed locally for me was test_select_prepended, which is not where I'm tweaking. I'll investigate these failures. Meanwhile, I'll close #61 and #62

@JimiRecard

Copy link
Copy Markdown
Contributor Author

Well, tests passed. Let me know what you think of the new tests and the way I used the classes at bootstrap5.py. Those are the points I'm most unsure about.

Cheers!

@smithdc1 smithdc1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, thank you. 👍

Although I've not yet sat down with this properly I've left a few initial comments.

Thanks again. :-)

Comment thread tests/test_layout_objects.py Outdated
Comment thread tests/test_layout_objects.py Outdated
Comment thread crispy_bootstrap5/bootstrap5.py Outdated
Comment thread crispy_bootstrap5/bootstrap5.py Outdated
Comment thread tests/test_layout_objects.py Outdated
Comment thread crispy_bootstrap5/bootstrap5.py Outdated

@smithdc1 smithdc1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Would you like to add a small note to the readme to document the new feature in this template pack? Happy to help, just let me know.

Comment thread crispy_bootstrap5/bootstrap5.py Outdated
Comment thread crispy_bootstrap5/bootstrap5.py Outdated
@JimiRecard

JimiRecard commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

Would you like to add a small note to the readme to document the new feature in this template pack?

Of course! I'll follow the floating labels note style

@smithdc1

Copy link
Copy Markdown
Member

Thanks for all your efforts here 🥇

@smithdc1 smithdc1 merged commit e1c1788 into django-crispy-forms:main Jun 18, 2021
@JimiRecard JimiRecard deleted the accordion-flush branch June 18, 2021 20:18
@smithdc1 smithdc1 added this to the Next Release milestone Jun 20, 2021
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