Code

Opened 7 years ago

Closed 4 years ago

#5609 closed (fixed)

newforms-admin: input fields do not regard field class

Reported by: anonymous Owned by: brosner
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 6 years ago.
Short version - most changes in trunk
trunk2.diff (29.1 KB) - added by oggie_rob 6 years ago.
newforms changes to support newforms-admin setting up widgets correctly

Download all attachments as: .zip

Change History (17)

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

  • Cc aloysius.prayitno@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by Xian

  • Owner changed from nobody to xian
  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 6 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 6 years ago by oggie_rob

Short version - most changes in trunk

Changed 6 years ago by oggie_rob

newforms changes to support newforms-admin setting up widgets correctly

comment:4 Changed 6 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 6 years ago by brosner

  • Keywords nfa-blocker added; newforms-admin removed

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

comment:6 Changed 6 years ago by Alex

  • Has patch set

comment:7 Changed 6 years ago by garcia_marc

  • milestone set to 1.0 alpha

comment:8 Changed 6 years ago by brosner

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 6 years ago by anonymous

  • Cc cmawebsite@… added

comment:10 Changed 6 years ago by brosner

  • Keywords nfa-blocker removed
  • milestone changed from 1.0 alpha to 1.0 beta
  • Owner changed from xian to brosner
  • Status changed from new to assigned

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 6 years ago by Alex

  • Version changed from newforms-admin to SVN

comment:12 Changed 6 years ago by jacob

  • Resolution set to duplicate
  • Status changed from assigned to closed

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

comment:13 Changed 5 years ago by jacmkno

  • Needs tests set
  • Resolution duplicate deleted
  • Status changed from closed to reopened

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 5 years ago by gwilson

  • milestone changed from 1.0 beta to 1.2

comment:15 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted
  • Resolution set to fixed
  • Status changed from reopened to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.