search

springload / madewithwagtail

A showcase of sites and apps made with Wagtail CMS, the easy to use, open source Django content management system
http://madewithwagtail.org
MIT License
84 stars 21 forks source link

Required fields unclear #84

Closed jennyadamsnz closed 6 years ago

jennyadamsnz commented 6 years ago

It's unclear which fields are required or optional. This is mentioned in a note in the "Profile description" field but this could be easily missed. I think it's good for there to be clarity here especially for regular submitters.

jennyadamsnz commented 6 years ago

I have added an asterisk to the required field's labels which I believe is common practice for indicating required fields in forms. Link to saved draft: http://madewithwagtail.org/admin/pages/5/view_draft/

thibaudcolas commented 6 years ago

👍 nice! If there is an asterisk, there should be a label somewhere on the form that states "* Required field". Please try and add it if that's not too complicated.

The only thing I would be concerned with is that changing all of the field's names breaks the form's results table / CSV export. Could you check on your local instance of the site and report back?

jennyadamsnz commented 6 years ago

export (9).pdf test was successful

jennyadamsnz commented 6 years ago

I have also added "An asterisk [*] indicates a required field" to the start of the form http://madewithwagtail.org/admin/pages/5/view_draft/

thibaudcolas commented 6 years ago

I understand that it would work with new submissions, but what happens of old ones that were made with different field names? It's a big price to pay to lose all of that history just for some label changes.

loicteixeira commented 6 years ago

Since the fields are marked as required in the admin, the template should be able to pick it up and add a required class to the field like we do on all other projects. If the basic form.as_p doesn't do it, we will have to loop over the fields and do it ourselves.

Edit: Alternatively it might be possible to overwrite def get_form_fields on core.forms.SubmitFormPage to dynamically update the returned fields and inject {'class': 'required'} to the widget attributes.

Edit2: Actually Wagtail already pass to the widget the required option along so there's probably no point overwriting get_form_fields.

Edit3: Anyway, this should be a code change, not a label change in the admin (in other words, we need to fix it not hack it).

loicteixeira commented 6 years ago

To my surprise, adding an asterisk to the label doesn't break the CSV Export. This is because it uses clean_name when saving to the database and exporting to the CSv.

However, note that the CSV Export has been "broken" on July 31st (see #98).

Anyway, as expressed before, I strongly believe this change should be a code change, not a hack in the admin. The above PR addresses that and I'll remove the * from the label once the PR is merged and deployed.

loicteixeira commented 6 years ago

I've deployed the code and while the CSS change is apparent (the new class exists), the form don't get the class applied.