Skip to content
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

Adding a class to required fields #50

Closed
wants to merge 4 commits into from
Closed

Adding a class to required fields #50

wants to merge 4 commits into from

Conversation

pwfff
Copy link

@pwfff pwfff commented Apr 24, 2012

I just had to do this for a project I'm working on, and I couldn't find an easy way to do it outside of these template files.

There's no existing framework in twitter-bootstrap for marking required fields so this may be outside of your project's scope, but I can't imagine it hurting anything.

Ideally, there'd be an easy way to toggle this, but I don't know your project well enough to make that kind of modification.

@jterrace
Copy link
Contributor

I think it would be nicer if it added something like class="required" instead of a style attribute. That would allow people to define whatever style they wanted for required fields.

@pwfff
Copy link
Author

pwfff commented Apr 24, 2012

This should be better. I moved it to the control-group (since the label isn't really what's 'required'), and changed the class from a generic 'required' to control-group-required, to match the other twitter-bootstrap class names.

In my project I'll do something like:

.control-group-required label { font-weight: bold; }
.control-group-required > input { background-color: whiteSmoke; }

<div class="control-group{% if errors %} error{% endif %}">
<label class="control-label" for="{{ bf_raw.id_for_label }}">{{ label }}</label>
<div class="control-group{% if errors %} error{% endif %}{% if field.required %} control-group-required{% endif %}">
<label class="control-label" for="{{ bf_raw.id_for_label }}">{{ label }}</label>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you meant to change the indentation of the label tag here. There's one of these below too, in prependedtext.html

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch... know of a vim plugin for detecting tab widths? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure. I use eclipse, so I just set project-specific settings based what's used in that project. HTML is typically indented by two spaces though because it tends to get nested quickly, so you might just want to set that as your default for HTML if possible.

@jterrace
Copy link
Contributor

lgtm now +1

@pthrasher
Copy link
Collaborator

Please see twbs/bootstrap#107

Best to mark fields as optional, and you should do this from the label text according to @mdo.

@pwfff
Copy link
Author

pwfff commented Apr 25, 2012

Did you mean twbs/bootstrap#207?

That doesn't really work for my project, which has many more optional fields than required ones (whether that's bad form design or not is beside the point). Plus, since it's an already existing (and quite large) Django codebase, I'd have to do something like extend the BootstrapModelForm constructor to manually append '(required)' to all of the help text, and that seems less than ideal...

Django already has the 'required' metadata stored. What's the harm in exposing it here? People don't have to take advantage if they don't want to.

@pwfff
Copy link
Author

pwfff commented Apr 30, 2012

Actually, it looks like this is already being included, but not used in the template. What's the meaning of this line: https://github.com/earle/django-bootstrap/blob/master/bootstrap/forms.py#L111

@pthrasher
Copy link
Collaborator

It appears that classes are built up, and are available within the template context. You could make a change to the templates to put the classes where you'd like them.

@pthrasher
Copy link
Collaborator

Please pull latest, merge in, and fix conflicts. I'll merge this in.

@pthrasher
Copy link
Collaborator

bump

@pthrasher
Copy link
Collaborator

I'd like to merge this in, but with no activity or responses, I'm closing it. Please request a new pull request if you'd like this merged in, and be sure to fetch latest, and resolve any potential conflicts.

@pthrasher pthrasher closed this Jun 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants