Repository navigation
Add wrapper=False to render a field without its wrapper - #882
shreyanshj10 wants to merge 7 commits into
Conversation
669fa6a to
0004da0
Compare
dyve
left a comment
There was a problem hiding this comment.
Verified on the branch rather than from the description. Two fields share one input-group with no wrapper divs, errors and help text survive on a bound form, and it propagates from bootstrap_form wrapper=False. The design answers the question #438 was parked on, and the docstring saying which classes are dropped and why is the part that makes it reviewable.
One problem with the documented example, suggested inline.
Bootstrap does not put <label> inside .input-group. Every child of an input group is laid out as a flex item, so a form-label between the controls is stretched into the row rather than sitting above it. Bootstrap's own examples keep the label outside the group. Someone copying this example gets labels wedged between the two inputs.
For a group holding several fields there is no sensible place for two separate visible labels, so the fields themselves should not render one. show_label='visually-hidden' would be the accessible answer, keeping the label for screen readers while leaving the layout alone, but it is currently a no-op on bootstrap_field: renderers.py line 392 tests if not self.show_label, and the string is truthy. I raised that on #879, which adds the same values to bootstrap_label and needs a one-line fix in the same place.
So there are two ways to sequence this:
- merge #879 with that one-line fix first, then use
show_label='visually-hidden'here, which is the accessible version; or - take the suggestion below now, using
'skip', and tighten it to'visually-hidden'once #879 lands.
Either way the example should not show labels inside the group. Flagging the accessibility tradeoff explicitly because 'skip' removes the label entirely, which is fine when your own markup supplies one and not fine when it does not.
| <div class="input-group"> | ||
| {% bootstrap_field form.field_a wrapper=False %} | ||
| {% bootstrap_field form.field_b wrapper=False %} | ||
| </div> |
There was a problem hiding this comment.
| <div class="input-group"> | |
| {% bootstrap_field form.field_a wrapper=False %} | |
| {% bootstrap_field form.field_b wrapper=False %} | |
| </div> | |
| <div class="input-group"> | |
| {% bootstrap_field form.field_a wrapper=False show_label='skip' %} | |
| {% bootstrap_field form.field_b wrapper=False show_label='skip' %} | |
| </div> |
The group needs a label of its own, outside it, since the fields no longer render one. Once #879 lands with the renderers.py fix, show_label='visually-hidden' is the better value here: it keeps a per-field label for screen readers without disturbing the flex layout.
| <div class="input-group"> | ||
| <label for="id_field_a" class="form-label">Field a</label> | ||
| <input type="text" name="field_a" class="form-control" id="id_field_a"> | ||
| <label for="id_field_b" class="form-label">Field b</label> | ||
| <input type="text" name="field_b" class="form-control" id="id_field_b"> | ||
| </div> |
There was a problem hiding this comment.
| <div class="input-group"> | |
| <label for="id_field_a" class="form-label">Field a</label> | |
| <input type="text" name="field_a" class="form-control" id="id_field_a"> | |
| <label for="id_field_b" class="form-label">Field b</label> | |
| <input type="text" name="field_b" class="form-control" id="id_field_b"> | |
| </div> | |
| <div class="input-group"> | |
| <input type="text" name="field_a" class="form-control" id="id_field_a"> | |
| <input type="text" name="field_b" class="form-control" id="id_field_b"> | |
| </div> |
Matching the input above, and matching what Bootstrap's input-group examples actually look like.
# Conflicts: # CHANGELOG.md
Bootstrap lays out every child of an input group as a flex item, so a field's label is stretched into the row instead of sitting above it, and Bootstrap's own examples keep the label outside the group. The example now renders the fields with show_label='skip' and says the group takes a label of your own, outside it.
…nal-wrapper # Conflicts: # CHANGELOG.md
|
The code is good and the tests pass locally, 202 of them. I checked the two behaviour claims in the new docs against this branch rather than taking them on trust, and both hold: One thing about the recommendation itself. A single label outside the group names only one of the fieldsThe example puts two fields in one group with Bootstrap's own answer for exactly this case, from <div class="input-group">
<span class="input-group-text">First and last name</span>
<input type="text" aria-label="First name" class="form-control">
<input type="text" aria-label="Last name" class="form-control">
</div>Each input carries its own
first = forms.CharField(widget=forms.TextInput(attrs={"aria-label": "First name"}))Could the example in The one-field case in your text is rightWorth keeping separate, because Bootstrap treats them differently. Where a group holds a single field, Bootstrap puts a <div class="col-auto">
<label class="visually-hidden" for="autoSizingInputGroup">Username</label>
<div class="input-group">
<div class="input-group-text">@</div>
<input type="text" class="form-control" id="autoSizingInputGroup" placeholder="Username">
</div>
</div>So "render the field without its label and write the label yourself, outside the group" is exactly right there. It is only the multi-field case where one label cannot cover them all. If the docs said which of the two you are in, the advice would carry further than the example. Nothing here blocks the code, it is all the one paragraph of prose. The branch is also |
…name 26.4 shipped while this was open and stamped the section the changelog entry was written into, so this takes main's CHANGELOG.md and re-adds the entry under the fresh `## Unreleased`. The input-group example also needed fixing. It told you to render several fields with `show_label='skip'` and write one label of your own outside the group, but a `<label for>` names one control, so every field after the first was left without an accessible name. What they fall back on is the `placeholder`, which this package sets from the label by default and which Bootstrap calls a last resort, since it disappears once the field has content. The example now gives each field its own `aria-label` and puts the shared, readable text in an `input-group-text`, which is what Bootstrap's own "Multiple inputs" example does. `bootstrap_field` takes no `attrs` argument, so the attribute goes on the widget, and the example shows that. Added the single-field case as a separate paragraph, because Bootstrap treats it differently: there it keeps a real `<label class="visually-hidden">` and places it before the group rather than inside it. The advice this example replaced was right for that case and only wrong for several fields. No change to the code in this pull request. Co-Authored-By: shreyanshj10 <shreyanshj10@users.noreply.github.com>
|
Picking this up so it does not sit any longer. The code is good and I am taking it as it stands; I have pushed one commit to your branch, since you left maintainer edits on. It changes no code, only the changelog placement and the docs example. Changelog. 26.4 shipped this morning and stamped the The input-group example. This is the point from my review on 7 October that had not been answered. The example told the reader to render several fields with The example now gives each field its own Your original advice was not wrong, it was right for a different case, so I kept it as its own paragraph: where a group holds a single field, Bootstrap keeps a real 202 tests pass, coverage holds at 100%, |
Both pull requests added an entry to the same `## Unreleased` section, which is the whole conflict. Kept all three, ordered as MAINTAINING.md asks, with the `show_label` fix above the two features. `renderers.py` and the template tags merged without conflict, but both pull requests changed the field renderer, so I checked the combination neither could have tested on its own. `wrapper=False` with `show_label='visually-hidden'` renders a `visually-hidden` label and no wrapper, and with `show_label='skip'` renders the widget alone. 210 tests pass and coverage holds at 100%.
Two fields cannot share one
input-grouptoday, because each field renders its own wrapperdivbetween the group and the widgets.wrapper_class=""does not help: it still renders<div class="">.This implements the wrapper-less mode from #438.
wrapper=Falserenders the label, widget, errors and help text with no element around them, so the fields can sit directly in aninput-groupof your own:The issue deliberately left the markup open, so this is a proposal as much as an implementation. Four decisions, each cheap to change if you want different output:
wrapperargument, defaultTrue, onbootstrap_fieldand, like every other renderer keyword, onbootstrap_formandbootstrap_formset. I did not usewrapper_class=Nonefor it: in a Django templateNoneresolves to''(the existingtest_wrapper_classshows this), so it cannot be told apart fromwrapper_class="", and that output stays exactly as it is. A boolean passed asFalsematchesshow_help,show_labelandserver_side_validation. I did not add aBOOTSTRAP5setting, because dropping the wrapper for every field in a project looks more like an accident than an intent, but it is a one-line addition if you would rather have one.invalid-feedbackstays a sibling of theis-invalidwidget, which is what Bootstrap wants inside an input group. Addinghas-validationto the group is left to you, as it already is when you write an input group by hand.wrapper_class, therowof the horizontal layout, andrequired_css_class/error_css_class/success_css_class. There is no element left to carry them, and the element that takes the wrapper's place is yours, so it can carry whatever it needs. Theis-valid/is-invalidclasses on the widget itself are untouched. The one exception isform-floating, which is functional rather than layout: withlayout="floating"the widget and its label keep their ownform-floatingelement, the same nesting used for floating labels inside an input group with add-ons.Rendering is byte-identical for anyone who does not pass
wrapper.Tests added in
tests/test_bootstrap_field_parameters.py: the default output is unchanged,wrapper=Falseon its own, two fields sharing oneinput-group, errors and help text surviving on a bound form with the indicator classes gone,form-floatingsurviving withlayout="floating", andwrapper=Falsereaching fields throughbootstrap_form. Docs andCHANGELOG.mdare updated.Fixes #438
I used an AI assistant while writing this. The four decisions above and the code are mine, and I can explain and defend either.