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

deform2: Support horizontal and inline form layouts. #187

Closed
wants to merge 5 commits into from

Conversation

dairiki
Copy link
Contributor

@dairiki dairiki commented Oct 1, 2013

With these changes, Form.bootstrap_form_style can be set to form-horizontal or form-inline to trigger bootstrap's alternative form layouts. (If I were starting from scratch, I would have put this in FormWidget.style, but
Form.bootstrap_form_style is how deform_bootstrap currently does it.)

For horizontal forms the columns widths are controlled by a new setting MappingWidget.column_classes (a two-tuple of CSS classes).

Within a horizontal form, if .column_classes is a false value, classes will be set in order to simulate the default "vertical" layout for that mapping (this involves setting both the label and control columns widths to col-xs-12 and omitting the control-label class from the control label.) By default MappingWidget.column_classes is None, while FormWidget.column_classes is not, so that the top level form will be laid out horizontally, while any nested mappings will use a faux-vertical layout. (This complication is necessary since, within a .form-horizontal, all .form_groups must be structured in the horizontal style (with columns).)

(Even if you don't like the rest of this, you should look at the first two commits.)

@mcdonc
Copy link
Member

mcdonc commented Oct 4, 2013

I wonder if you could submit the first 2 commits as a separate pull request, because those seem like no-brainers. The form-inline feature may require some back and forth; there is no reason we have to do it like deform_bootstrap did it at all.

@mcdonc
Copy link
Member

mcdonc commented Oct 4, 2013

FWIW, I've made it so that if a top-level schema has a custom FormWidget, its style will be used as the form style. I took out bootstrap_form_style, replacing it with the style of the widget. It may be a bit indirect to have to assign a FormWidget to a schema to allow there to be inline styling, but it's consistent with how the rest of the framework works. We might choose a way to shortcut that, I suppose, I don't want to do anything (more) inconsistent without thinking about it., and there's so much left to do to push this branch out the door that I don't currently want to spend much time on it.

The changes I made a few minutes ago are going to cause this patch (and possibly your others) some pain, apologies. I'd still like to get in the first two commits, if you have the gumption to reapply them against the changed code.

I'd be all for renaming Form.bootstrap_form_style to something shorter.
(Also, I think it makes more sense as an attribute of FormWidget rather
than of Form.) Deform_bootstrap uses Form.bootstrap_form_style,
however, so compatibility considerations may be warranted.
(to prevent lonely red asterisk)
@dairiki
Copy link
Contributor Author

dairiki commented Oct 4, 2013

Rebased to current tip, and fixed to look at form.widget.css_class rather than form.bootstrap_form_style.

@dairiki
Copy link
Contributor Author

dairiki commented Oct 7, 2013

This is currently broken in the case of sequences of mappings. I'm not sure how best to fix it.

The issue is that mapping_item.pt needs to know whether the top-level form is in horizontal mode or not. (The necessary DOM structure is different when inside a .form-horizontal.) Currently (in this branch), mapping_item.pt finds the form by using the new Field.get_root() method. The problem comes when SequenceWidget.prototype() or SequenceWidget.serialize() renders a clone of the sequence item. That clone has no .parent so there is no way to find the top-level form field.

Here are some ways I can think of to fix this, listed in order of preference. (Or hopefully there's a better solution?)

  1. Change Field.clone() to preserve the parent. Unfortunately, the docstring for Field.clone explicitly states that the parent of the clone will be set to None (f4869e1). Is there a reason why this is so? (It looks like .clone() is used only by SequenceWidget, and it seems SequenceWidget would benefit from having parent-preserving clones.)
  2. Have the SequenceWidget methods explicitly restore the parent of clones. Currently there is no API for this — one must set ._parent directly.
  3. [yuck] If there is a real reason why clones should not have parents, a Field.form attribute (implemented via weakref) referencing the form to which the field belongs could be added. This attribute would be preserved by .clone().
  4. [probably too ugly to consider] Explicitly pass the top-level form to the template for each field/subfield.

The reasons for this (and some alternatives) are listed at
Pylons#187 (comment)
@tisdall
Copy link
Contributor

tisdall commented Sep 15, 2015

sorry, guys.. I submitted an issue about the deform2 branch being dead and confusing so @tseaver deleted it. Can you resubmit this on the master branch?

dairiki added a commit to dairiki/deform that referenced this pull request Sep 16, 2015
The reasons for this (and some alternatives) are listed at
Pylons#187 (comment)
@stevepiercy stevepiercy added this to the 3.0.0 milestone Jun 22, 2020
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.

5 participants