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

Create a bootstrap modal layout object #1204

Merged
merged 22 commits into from Jan 24, 2022

Conversation

nsandler1
Copy link
Contributor

@nsandler1 nsandler1 commented Jan 13, 2022

Introduces bootstrap modals into crispy forms as a layout object.

Supported by Bootstrap v3+

@smithdc1
Copy link
Member

Thanks for this, it looks like a nice addition.

Could I ask you to have a look at updating the docs, and adding some tests?

Here's a commit that shows how we're currently writing tests. The aim is with this style that it makes them easier to test than lots of HTML in Python strings.

Do let me know if you need any help with these.

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Merging #1204 (88388cc) into main (e84b43f) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
+ Coverage   97.35%   97.38%   +0.03%     
==========================================
  Files          23       23              
  Lines        2909     2950      +41     
  Branches      327      328       +1     
==========================================
+ Hits         2832     2873      +41     
  Misses         39       39              
  Partials       38       38              
Flag Coverage Δ
unittests 97.38% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
crispy_forms/bootstrap.py 91.58% <100.00%> (+0.72%) ⬆️
crispy_forms/tests/test_layout_objects.py 98.50% <100.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e84b43f...88388cc. Read the comment docs.

@nsandler1
Copy link
Contributor Author

@smithdc1 Of course! I wanted to hold off on updating the docs and writing tests until my pull request got attention. Thanks for the example.

@nsandler1
Copy link
Contributor Author

Modals are only supported by bootstrap 3 and 4 with no changes between the versions that would cause incompatibility. f1a8382 only tests bootstrap3 modals and the assumption is a bootstrap4 modal would work the same. If a test for bootstrap4 is requested I'm more than willing to add one, though it would be the exact same test as bootstrap3, just with the bootstrap4 decorator.

@nsandler1
Copy link
Contributor Author

@smithdc1 I'm a bit stuck with how to pass the Python 3.7 check. Any ideas?

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.

Ah yes, the whole space "thing" with Django 2.2. Not long left to support this version.

Adding a couple of spaces to the tests should fix it.

This is now top of my list to have a look at, looks great a a first glance!

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 again for the PR! 🏅

I've got a few notes, mostly nits and/or questions about some edge cases. Appreciate your thoughts on these.

crispy_forms/bootstrap.py Outdated Show resolved Hide resolved
crispy_forms/bootstrap.py Show resolved Hide resolved
docs/layouts.rst Outdated Show resolved Hide resolved
docs/layouts.rst Outdated Show resolved Hide resolved
docs/layouts.rst Outdated Show resolved Hide resolved
crispy_forms/tests/test_layout_objects.py Show resolved Hide resolved
docs/layouts.rst Show resolved Hide resolved
crispy_forms/bootstrap.py Outdated Show resolved Hide resolved
crispy_forms/bootstrap.py Outdated Show resolved Hide resolved
nsandler1 and others added 21 commits January 22, 2022 17:43
…strap_modal_no_kwargs.html

Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
…strap_modal_no_kwargs.html

Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
Co-authored-by: David Smith <39445562+smithdc1@users.noreply.github.com>
@smithdc1
Copy link
Member

Ok, tests are passing. Is there anything else before I merge?

@smithdc1 smithdc1 merged commit 33c9488 into django-crispy-forms:main Jan 24, 2022
@smithdc1
Copy link
Member

@nsandler1 thanks for the contribution!

@nsandler1
Copy link
Contributor Author

nsandler1 commented Jan 24, 2022

@smithdc1 Nope, looks good to me!

No problem!

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.

None yet

3 participants