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

Fixed offset class of checkbox in form horizontal #51

Merged
merged 5 commits into from
Jun 3, 2021

Conversation

lauri-openscop
Copy link
Contributor

A tricky way to apply the right offset class on checkbox field whitout touching the core application..

A tricky way to apply the right offset class on checkbox field whitout touching the core application..
@lauri-openscop lauri-openscop changed the title Fixed compatibility with bootstrap5 template Fixed offset class of checkbox in form horizontal May 22, 2021
@smithdc1
Copy link
Member

Thanks! Glad you found a way of including it in this template pack. 🏅

Could I ask you to add tests for this? See the link below for how we are writing new tests.

#44

Update test_label_class_and_field_class_bs5_offset_when_horizontal to match with the current bootstrap 5 offset
@lauri-openscop
Copy link
Contributor Author

lauri-openscop commented May 25, 2021

I have updated test_label_class_and_field_class_bs5_offset_when_horizontal to match with the current offset method. Is that good for you ?

@smithdc1 smithdc1 closed this May 31, 2021
@smithdc1 smithdc1 reopened this May 31, 2021
@smithdc1
Copy link
Member

smithdc1 commented Jun 1, 2021

Thanks for this. I think this is close but it doesn't work where the number of columns is two digits. i.e. col-lg-12 would become offset-lg-1.

I was wondering if we could refactor bootstrap_checkbox_offsets in core to be a list/dict(?) of size and number of columns. We could then build the string in the template rather than code as the particular format here seems to change with each major bootstrap release.

What do you think?

@lauri-openscop
Copy link
Contributor Author

lauri-openscop commented Jun 1, 2021

My bad, it was cause by the slice. I didn't think about two digit columns. It should work now..
I think that if we have to change the core application the best would be to change helper.py like i said in #49 (comment) or something like that. What do you think ?

@smithdc1
Copy link
Member

smithdc1 commented Jun 3, 2021

I think that github asking folk to approve each test run is a bit tiresome (I mean, once would be ok for this PR, but I've pressed it countless times for this PR alone as we've been itterating through the problem!).

As for this patch itself I think it's fine. Let's have this. 👍

@smithdc1 smithdc1 merged commit 80a89e4 into django-crispy-forms:main Jun 3, 2021
@smithdc1 smithdc1 changed the title Fixed offset class of checkbox in form horizontal Fixed offset class of checkbox in form horizontal Jun 3, 2021
@smithdc1 smithdc1 added this to the Next Release milestone Jun 3, 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