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 support for accordion-flush and accordion always open #63

Merged
merged 1 commit into from
Jun 18, 2021
Merged

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

merged 1 commit into from
Jun 18, 2021

Conversation

JimiRecard
Copy link
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
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
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!

Copy link
Member

@smithdc1 smithdc1 left a comment

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. :-)

tests/test_layout_objects.py Outdated Show resolved Hide resolved
tests/test_layout_objects.py Outdated Show resolved Hide resolved
crispy_bootstrap5/bootstrap5.py Outdated Show resolved Hide resolved
crispy_bootstrap5/bootstrap5.py Outdated Show resolved Hide resolved
Copy link
Member

@smithdc1 smithdc1 left a comment

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.

crispy_bootstrap5/bootstrap5.py Outdated Show resolved Hide resolved
crispy_bootstrap5/bootstrap5.py Outdated Show resolved Hide resolved
@JimiRecard
Copy link
Contributor Author

JimiRecard commented Jun 17, 2021

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
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