Opened 9 years ago

Closed 7 years ago

#5609 closed (fixed)

newforms-admin: input fields do not regard field class

Reported by: anonymous Owned by: Brian Rosner
Component: contrib.admin Version: master
Severity: Keywords: length input class
Cc: aloysius.prayitno@…, cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

I tried the newforms-admin branch, and noticed input field length for a SmallInteger field is wider than the trunk. Below is excerpt from the html source from newforms-admin branch and trunk. The one from newforms-admin is missing class=... etc.

newforms-admin:

<label for="id_treatment_visits" class="required">Treatment visits:</label>
<input type="text" name="treatment_visits" value="19" id="id_treatment_visits" />

trunk:

<label for="id_treatment_visits" class="required">Treatment visits:</label> 
<input type="text" id="id_treatment_visits" class="vSmallIntegerField required" name="treatment_visits" size="5" value="19" maxlength="5" />

Attachments (2)

na2.diff (1.9 KB) - added by oggie_rob 9 years ago.
Short version - most changes in trunk
trunk2.diff (29.1 KB) - added by oggie_rob 9 years ago.
newforms changes to support newforms-admin setting up widgets correctly

Download all attachments as: .zip

Change History (17)

comment:1 Changed 9 years ago by awi <aloysius.prayitno@…>

Cc: aloysius.prayitno@… added

comment:2 Changed 9 years ago by Xian

Owner: changed from nobody to xian
Triage Stage: UnreviewedAccepted

Thanks for reporting this, please let me know if any other field types are displaying incorrect widgets.

comment:3 Changed 9 years ago by oggie_rob

Working on this for the sprint. I'll be working off trunk and changing the db.models.fields.formfields() functions and probably some newforms.fields areas also. This will also result in changes to form_for_model/instance that I believe will be preferred (since things like class="required" is currently not being served up via form_for_model).

Changed 9 years ago by oggie_rob

Attachment: na2.diff added

Short version - most changes in trunk

Changed 9 years ago by oggie_rob

Attachment: trunk2.diff added

newforms changes to support newforms-admin setting up widgets correctly

comment:4 Changed 9 years ago by oggie_rob

This code is split two ways. Most of the changes I made were in newforms, so that form_for_model will return an appropriate widget. For example, a CharField model field will return a TextInput with class="required" (if blank=False), length="30" and maxlength according to the model. These changes then support features in newforms-admin pretty easily.

I updated some tests but ran out of time. Hopefully I can get to them later, but if the target branch is in question it would make sense to delay. Certainly there are plenty of tests that fail currently.

Finally one area that was a little funky... forms.Field uses a TextInput as a default widget, however the number of arguments in the __init__ function are rather small. It is a bit strange because typically you may want to set "size" and "maxlength" attributes of a TextField. Anything that extends Field misses out on those, however, and it is better to extend CharField. My feeling is that it would have been cleaner to not have a default widget.

comment:5 Changed 9 years ago by Brian Rosner

Keywords: nfa-blocker added; newforms-admin removed

This breaks current admin functionality. I am tagging with nfa-blocker.

comment:6 Changed 9 years ago by Alex Gaynor

Has patch: set

comment:7 Changed 8 years ago by Marc Garcia

milestone: 1.0 alpha

comment:8 Changed 8 years ago by Brian Rosner

Marked #7710 as a duplicate. We need to go through and find all the instances where there is a missing CSS class on a widget.

comment:9 Changed 8 years ago by anonymous

Cc: cmawebsite@… added

comment:10 Changed 8 years ago by Brian Rosner

Keywords: nfa-blocker removed
milestone: 1.0 alpha1.0 beta
Owner: changed from xian to Brian Rosner
Status: newassigned

This ticket is going to require a bit of work. alpha will have to be released with this as a known issue in the admin. Bumping to 1.0-beta.

comment:11 Changed 8 years ago by Alex Gaynor

Version: newforms-adminSVN

comment:12 Changed 8 years ago by Jacob

Resolution: duplicate
Status: assignedclosed

#8163 has a better (looking, mostly) patch.

comment:13 Changed 8 years ago by jacmkno

Needs tests: set
Resolution: duplicate
Status: closedreopened

I don't think the #8163 has a solution for this that thicked was supposed to be fixed but it still does not work in the current dev version.
This ticket has to be re-opened.

comment:14 Changed 8 years ago by Gary Wilson

milestone: 1.0 beta1.2

comment:15 Changed 7 years ago by James Bennett

milestone: 1.2
Resolution: fixed
Status: reopenedclosed

So far as I'm aware, this was fixed a long time ago.

Note: See TracTickets for help on using tickets.
Back to Top