-
Notifications
You must be signed in to change notification settings - Fork 316
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
Prevent having "None" in the placeholder #591
Conversation
Prevent having "None" in the placeholder
Codecov Report
@@ Coverage Diff @@
## master #591 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 8 8
=========================================
Hits 8 8 Continue to review full report at Codecov.
|
Maybe the change should happen in line 83 : |
@Benbb96 thanks for the contribution. I am not quite certain about this change though. Why are is your fields |
Okay so here is an example of my code when class OrderForm(forms.ModelForm):
class Meta:
model = Order
fields = ('status', 'customer_contacts')
widgets = {
'status': forms.Select(attrs={'class': 'form-control'}),
'customer_contacts': ModelSelect2MultipleWidget(
model=User,
search_fields=['username__icontains', 'first_name__icontains', 'last_name__icontains']
),
} And next is the way I have it working without applying the patch I submitted : class OrderForm(forms.ModelForm):
customer_contacts = forms.ModelMultipleChoiceField(
queryset=User.objects.all(),
widget=ModelSelect2MultipleWidget(
model=User,
search_fields=['username__icontains', 'first_name__icontains', 'last_name__icontains']
)
)
class Meta:
model = Order
fields = ('status', 'customer_contacts')
widgets = {
'status': forms.Select(attrs={'class': 'form-control'})
} Why do I have to explicitly override the field Note : I didn't have this issue before this commit. |
Hi @Benbb96, thanks for responding so quickly. I had a look at the Django code, to figure out why the label is django-select2/django_select2/forms.py Line 83 in 39ab326
Can you test if replacing the line with default_attrs['data-placeholder'] = self.empty_label or "" solves your problem? If so, I am happy to accept this as a patch. Best |
Hi @codingjoe, You're right, if I set my field as required, then the placeholder isn't I've tested with your proposal and it also works so I updated my code on my fork. |
Just update it and maybe add test if possible. Ping me when you are done and I'll review an release it 👍 |
Hi @codingjoe I added a test and pushed it onto my fork, but I'm not sure if you are able to see it from here since the pull request is closed ? I've added my test into the |
Released in 7.2.1 🎉 |
I had None set as a placeholder on my ModelSelect2MultipleWidget because I initialize the widget through the Meta class of my form and I don't explicitly set the empty_label on my field.
So I think it should check if the empty_label is set before assigning it to the placeholder.
This issue is linked to #565