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

Custom select #1091

Merged
merged 8 commits into from
Dec 16, 2020
Merged

Custom select #1091

merged 8 commits into from
Dec 16, 2020

Conversation

cpina
Copy link
Member

@cpina cpina commented Dec 11, 2020

Add class="custom-control custom-select" for the select widgets.

Before (see the arrow at the end):
select

After (Bootstrap4 arrow):
custom_select

When it has been seen it cannot be unseen!

In django-crispy-forms Bootstrap4 there is no specific template for the select widget: it relies on the Django template. This is good (one less think to maintain) but conceptually I think that would prefer to avoid having Bootstrap4/template-pack specific code in crispy_forms_field.py: "custom-control custom-select" could be moved into a new template for "select" like it's done for other widgets. On the other hand it's handy to not have the template and piiggyback Django select (django/forms/templates/django/forms/widgets/select.html) (other possible refactorings would be possible). Perhaps when working on Bootstrap5 it would be a good moment to re-consider this.

The Bootstrap4 (or Bootstrap3) code in crispy_forms_field.py dates from 2013.

This is consistent with checkboxes, radiobuttons and many other fields.
…et Bootstrap4 custom-control

It also updates the documentation on File widget: the clearable file
widget in recent django-crispy-forms looks similar to the non-clearable
custom widget.
@codecov-io
Copy link

codecov-io commented Dec 11, 2020

Codecov Report

Merging #1091 (70fe6e6) into master (71a5058) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1091      +/-   ##
==========================================
+ Coverage   97.20%   97.22%   +0.01%     
==========================================
  Files          24       24              
  Lines        2791     2811      +20     
  Branches      243      243              
==========================================
+ Hits         2713     2733      +20     
  Misses         38       38              
  Partials       40       40              
Flag Coverage Δ
unittests 97.15% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
crispy_forms/templatetags/crispy_forms_field.py 94.54% <100.00%> (+0.15%) ⬆️
crispy_forms/tests/forms.py 100.00% <100.00%> (ø)
crispy_forms/tests/test_layout.py 99.72% <100.00%> (+0.01%) ⬆️

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 71a5058...70fe6e6. Read the comment docs.

@cpina
Copy link
Member Author

cpina commented Dec 12, 2020

With the latest commit (12th Dec): the logic to fix this is in the templates.

This has a problem: the custom-select select does not work for Prepended/Appended texts. This is not only for the select but for other widgets. See for more details #1093 .

So this case works fine:
select-good

But this case is not working yet:
select-bad

But since this is related to any widget I would plan a solution in #1093

crispy_forms/templatetags/crispy_forms_field.py Outdated Show resolved Hide resolved
crispy_forms/templates/bootstrap4/field.html Outdated Show resolved Hide resolved
crispy_forms/templates/bootstrap4/field.html Outdated Show resolved Hide resolved
crispy_forms/tests/test_layout.py Show resolved Hide resolved
@@ -159,23 +165,17 @@ All previous, ``bootstrap`` (version 2 and 3) attributes are also settable in bo
.. |customfile| image:: images/custom_file_field.png
:width: 300px
:align: middle
.. |clearablefile| image:: images/clearable_file.png
Copy link
Member

Choose a reason for hiding this comment

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

Dropping clearable file from here is an unrelated change?

Copy link
Member Author

@cpina cpina Dec 15, 2020

Choose a reason for hiding this comment

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

Edit: oh wait, in any case it intended to drop the file field custom clearable that I had intended to drop since it looks the same as the file field custom. I'll build the docs and see that all looks good and take new screenshots if needed.

Previous message:
Yes (I noted this in the commit, perhaps I should move it to a PR): cpina@c134023

The file field custom clearable png is, at the moment, broken in the documentation: https://django-crispy-forms.readthedocs.io/en/latest/form_helper.html?bootstrap-4-helper-attributes#bootstrap-4-helper-attributes

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping clearable file from here is an unrelated change?

I've created a branch minimising this change: https://github.com/cpina/django-crispy-forms/tree/custom-select-with-minimal-docs-change . I might have got a bit carried on the other day. I can make the other changes (delete the clearable one maybe, re-order the table so it matches the text) in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

The clearable file also looks awful on rtd, I'm not sure it's that helpful.

:width: 300px
:align: middle
.. |customradio| image:: images/custom_radio.png
.. |customselect| image:: images/custom_select.png
Copy link
Member

Choose a reason for hiding this comment

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

/home/pi/Projects/django-crispy-forms/docs/form_helper.rst:156: WARNING: image file not readable: images/select.png
/home/pi/Projects/django-crispy-forms/docs/form_helper.rst:159: WARNING: image file not readable: images/custom_select.png

I can't seem to build the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll amend this (I missed adding some files). How are you building the docs? I can't see in the documentation or in the .github action.

Copy link
Member Author

Choose a reason for hiding this comment

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

cd docs
make html

I might add this in the docs (or read them better if it's there)

@smithdc1
Copy link
Member

Hi Carles, thanks for looking at this, this is a nice improvement. I've left a few small comments.

I think we should take this as a standalone item which covers the most likely use case. We can then look at a select within a append / prepended wrapper as a separate item?

I'd be happy to push a release with just this change.

@cpina
Copy link
Member Author

cpina commented Dec 15, 2020

Hi Carles, thanks for looking at this, this is a nice improvement. I've left a few small comments.

I think we should take this as a standalone item which covers the most likely use case. We can then look at a select within a append / prepended wrapper as a separate item?

I'd be happy to push a release with just this change.

I'm also happy to address the pre/post append select issues as part of #1093 . I think that pre/post append might need bigger changes and longer discussion.

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 to me! 🎉

@cpina cpina merged commit 395d583 into django-crispy-forms:master Dec 16, 2020
@smithdc1 smithdc1 added this to the Next Release milestone Jan 24, 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.

None yet

3 participants