jrief / django-angular

Let AngularJS play well with Django
http://django-angular.awesto.com/
MIT License
1.23k stars 294 forks source link

Change the widget css class using a dictionary similar to field_css_classes #215

Closed jshridha closed 8 years ago

jshridha commented 8 years ago

This is a merge request to allow changing the css class for widgets using a dictionary key similar to field_css_classes. I need this to be able to add a datepicker class to certain form fields.

adrienbrunet commented 8 years ago

I'm always saying it to new contributors: thanks a lot for your pull request. Right now, I can't merge it as is. Could you provide documentation for this feature? What about a test and/or an example? If you don't have time for that, I'll do it but it won't be really soon. Again, thanks! :+1:

jrief commented 8 years ago

Thanks @jshridha for this PR. You you please elaborate what kind of problem you want to resolve. Maybe there is an easier solution. Also, please add or modify a unit test to check your code against possible future regressions.

jshridha commented 8 years ago

@jrief I'm trying to add a class only to specific widgets. Currently the widget_css_classes field applies a class to every widget, but I only want to apply a class to certain widgets similar to how field_css_classes work.

If there is an easier solution, I'm all ears :)

I'll work on adding in a unit test and some documentation if you feel the feature is useful. I ended up no longer needing this for my project, but others might find this useful as well.

adrienbrunet commented 8 years ago

This is not such a bad idea. It does more or less the same thing that the field_css_classes magic does and could be usefull. BUT, why should we care adding this feature? You could add in your form definition:

def __init__(self, *args, **kwargs):
    super().__init__(self, *args, **kwargs)
    self.fields['my_field'].widget.attrs['class'] = 'my_new_class'

Or you could define a custom widget. I don't feel confortable adding new features which are not really the django encouraged way of doing things. What do you both think ?

jshridha commented 8 years ago

I completely agree. If there is a more Django centric way of doing it, then that is the right way of doing it.

I tried adjusting the widget attribute of the method class within the form, but those classes were getting overwritten by djangular's methods within the bootstrap mixin. I didn't try overriding the init method, I'll try that and if that works it's definitely the preferred solution. On Dec 24, 2015 12:27 PM, "Adrien Brunet" notifications@github.com wrote:

This is not such a bad idea. It does more or less the same thing that the field_css_classes magic does and could be usefull. BUT, why should we care adding this feature? You could add in your form definition:

def init(self, _args, _kwargs): super().init(self, _args, _kwargs) self.fields['my_field'].widget.attrs['class'] = 'my_new_class'

Or you could define a custom widget. I don't feel confortable adding new features which are not really the django encouraged way of doing things. What do you both think ?

— Reply to this email directly or view it on GitHub https://github.com/jrief/django-angular/pull/215#issuecomment-167139812.

adrienbrunet commented 8 years ago

Let us know if the override of __init__ works. Then, we can close this. (I'm doing a Christmas :christmas_tree: cleaning hehe)

jshridha commented 8 years ago

I'm currently in the car driving to Christmas, but found a chance to try it out :). It still gets overwritten. The class attr isn't set by the end of the init method, so I just need to find the right method in the mixin to hook into. On Dec 24, 2015 12:53 PM, "Adrien Brunet" notifications@github.com wrote:

Let us know if the override of init works. Then, we can close this. (I'm doing a Christmas [image: :christmas_tree:] cleaning hehe)

— Reply to this email directly or view it on GitHub https://github.com/jrief/django-angular/pull/215#issuecomment-167141507.

adrienbrunet commented 8 years ago

Hahaha you rock! :guitar: If you find it, please let us know! But maybe you have better things to do today, enjoy ! :cake: :christmas_tree: :santa:

jshridha commented 8 years ago

I was just on the edge of finding the bug and ran out of battery on my laptop. There's something that happens very early on, even before looking at widget_css_classes that ignores or deletes the class key in the widget's attrs dict. It may be coming in from the ngboundfield mixin, but I'll have to search for it later. On Dec 24, 2015 1:08 PM, "Adrien Brunet" notifications@github.com wrote:

Hahaha you rock! [image: :guitar:] If you find it, please let us know! But maybe you have better things to do today, enjoy ! [image: :cake:] [image: :christmas_tree:] [image: :santa:]

— Reply to this email directly or view it on GitHub https://github.com/jrief/django-angular/pull/215#issuecomment-167143280.

adrienbrunet commented 8 years ago

You' re awesome, merry christmas :santa: Try to work plugged in next time haha

jshridha commented 8 years ago

I was able to find the bug that was causing the class attr to be overwritten and I updated the pull request. Hopefully it passes the travisci checks!

adrienbrunet commented 8 years ago

:fireworks: Yay, that works. Great job =)

adrienbrunet commented 8 years ago

Hey, I reverted the PR. In fact, is attrs['class'] always defined? If attrs['class'] is not defined, It wont work. I merged it too quickly. There are no tests for that so we need to be very carefull.

jshridha commented 8 years ago

Before I updated the pull request, I checked to see if there were any cases where the attrs['class'] was not defined. I couldn't get any, but I decided to update the pull request to check anyway just in case. It is now updated, but it looks like I have to make a new pull request since this one was reverted. Do you know a way to update the pull request without having to make a new one?

adrienbrunet commented 8 years ago

Unfortunately, I don't know how to do it without creating a new one. :sob: