I'll quote part of a comment from #116939: Add a Cancel link on forms that triggered this one:
Form buttons should be wrapped in an additional container to achieve a consistent styling, but also allow for a "Cancel" link, as contained in confirmation forms (which use additional tweaks to get the styling right). Wrapping form buttons into a parent element 'actions' (or similar) also allows for additional styling, for example, applying CSS Sliding Doors to buttons and links to achieve a more modern look.
(read #43 to #47 in the other issue for the full backstory, it has sun, quicksketch and me agreeing enthousiastically :-)
Comment | File | Size | Author |
---|---|---|---|
#59 | drupal.form-actions.59.patch | 51.77 KB | sun |
#57 | drupal.form-actions.57.patch | 51.74 KB | sun |
#50 | drupal.form-actions.50.patch | 32.5 KB | sun |
#49 | drupal.form-actions.49.patch | 31.47 KB | sun |
#48 | drupal.form-actions.48.patch | 1.63 KB | sun |
Comments
Comment #1
sunsubscribing
Comment #2
yoroy CreditAttribution: yoroy commentedasked around a bit on how this would work, chx and yhahn replied:
currently a form element has no knowledge of its children
we can add a #type 'buttom-wrapper'
and then have $form['buttons']['#type'] = 'button-wrapper';
and put all the relevant submit buttons under that element
and then $form['buttons']['submit'] = array(....)
if you hit system_settings_form, node / add / edit, and a few other you've covered a good portion of core forms
and then you can theme_button_wrapper as much as you want,.
Comment #3
mortendk CreditAttribution: mortendk commentedThis would be pretty sweet and we could then remove the "unnecessary" div inside the forms ( http://drupal.org/node/495480)+ would give us some better control over the submit, delete cancel button (if they are put in the bottom of the page)
+1
Comment #4
Stefan Nagtegaal CreditAttribution: Stefan Nagtegaal commentedI totally agree..
Let's combine it together with #165567: Usability: Float default submit buttons to the right and the other to the left which I submitted ages ago. Let the buttons which reflect the default action of the page ("Save $foo", etc) on the right (when the theme is LTR, or the other way around when it's RTL) and the other buttons on the left (like "Cancel", or "Reset to defaults)'.
Comment #5
sunSorry, but no. Please stay on focus. Form buttons can appear everywhere in a form, see admin/settings/performance as an example. This issue should focus on wrapping form buttons into a container - to allow for consistent styling. If you mix this issue with any other, I can promise you it won't make it into D7.
#theme_wrapper might work out. Maybe it also works with some magic applied, but probably not. (see also example hack in #116939-48: Add a Cancel link on forms)
Ultimately, a consistent wrapper for form buttons will probably depend on developers implementing it (i.e.
'#theme_wrapper' => 'form_actions'
). However, to do that, module developers might have to rework their form structure - referring to aforementioned hack again:becomes:
Comment #6
JacineThis would be awesome!
Comment #7
sunIntroducing a new tag for feature freeze: API clean-up.
Comment #8
RobLoach#557272: FAPI: JavaScript States system introduces a "container" type. So instead, you could have:
Comment #9
gausarts CreditAttribution: gausarts commentedLooking for this feature. Thanks
Comment #10
RobLoach#622432: DX: Start using the "container" form type demonstrates use of this.
Comment #11
zroger CreditAttribution: zroger commentedI like rob's approach of using a simple container.
For another use case...
In jQuery UI dialog, you can specify a buttons object, which the Dialog will display in a consistent area, at the bottom left of the dialog. Having a consistent wrapper would allow you to hide the default buttons and delegate the submission of the form to the jQuery UI dialog buttons. Without a consistent wrapper though, there is no way to do this reliably.
Checkout the d6 version of dialog api (http://drupal.org/project/dialog) for an example of this. In order to achieve this in d6 though, i've resorted to form_altering a class onto the buttons that I want to use.
Comment #12
RobLoachThis adds a container around all form buttons with a class named "form-actions". We need this in, badly. Those lost form buttons drive me nuts. I'm probably missing some buttons here, so some reviews would be nice.
Comment #14
sunyay, thanks!
That was still quite inconsistent though... So let's go with this instead? :)
Comment #15
sunThis one should fix the exceptions.
Comment #16
sunSo what's left?
We want to consolidate extra DIVs like this one. I.e. we can apply the #id and extra .container-inline class to the container's #attributes already :)
Aside from that, this looks very RTBC-ish!
This review is powered by Dreditor.
Comment #17
RobLoachTo make this change, we needed to add support for the "#id" property....
I also noticed we were missing some buttons in Locale module, so I added those.
Comment #18
sunMerged that last patch with mine, as we worked on this at the same time.
Additionally:
- Removed totally senseless theme functions for filter forms.
- Applied consistent pattern: One-line syntax for 'actions' if it's the regular thing, multi-line if there are special tweaks contained (such as .container-inline)
Furthermore, I double-checked the actual output of most forms.
Looks ready to fly for me.
Comment #19
sunVery interesting effect, when you remove a theme function without the registry entry in hook_theme(). Is that a bug elsewhere?
Anyway, this one fixes the notices.
Comment #20
sunComment #21
RobLoachDefinitely like the clean up of the theme functions. Unless there's anything else missing from this patch, I'm setting this to RTBC.
Comment #22
yoroy CreditAttribution: yoroy commentedI'm just glad I opened this issue :-)
Sure hope to see this committed.
Comment #23
sunThis patch is backed 100% by themers.
Yes, we want. :)
Comment #24
dman CreditAttribution: dman commentedNice. This is a sensible improvement
Comment #27
sunRe-rolled against HEAD.
Comment #28
webchickHm. This came after code freeze and is not fixing a critical bug. OTOH, I noticed Dries committed a patch that added such a container to node forms, so I suppose we could do this too, to be consistent....
Ok. Committed to HEAD.
Needs to be documented in the module upgrade guide as a new standard.
Comment #29
sunThank you! This is really exciting and will drastically improve #D7TX! TX? Theming experience! ;)
Comment #30
jbrown CreditAttribution: jbrown commentedOn admin/content 'Filter' now has a container, but 'Update' does not.
I think buttons at the bottom of a form should have a different class so they can have a margin-top.
Adding margin-top to buttons within the form screws up the layout.
Comment #31
jide CreditAttribution: jide commentedThere may be some more buttons to wrap, searching for $form['submit'] or $form['save'] in all files reveals a bunch of them.
But there may be reasons for those to have been left unwrapped ?
Comment #32
sunNope, not intentionally. It's also possible that forms were changed in the meantime, without knowledge of this new standard here.
Comment #33
TwoDSubscribing, how can I help?
Btw,
grep -Ern "\['(submit|save|preview|delete|undo|reset|)'\] = " . | grep -Fv -e "['actions']"
gives the below list of hits (minus a comment or two). Should all those be wrapped?Comment #34
TwoDWhoa, list was a lot longer than I thought. Attaching instead.
Comment #35
jide CreditAttribution: jide commentedWhen that gets in, what do you think about making form['actions'] weight to 100 everywhere by default ?
This has been done for system_settings_form() in #645758: system_settings_form() should set a high weight for buttons, so it would be the good place for that.
Comment #36
seutje CreditAttribution: seutje commentedsubscribe
Comment #37
sunI have absolutely no idea how all of those were able to slip through.
Attached patch should fix all of them.
Comment #38
sunSorry, one wrong change in menu.admin.inc.
Comment #40
RobLoach#38: drupal.form-actions.38.patch queued for re-testing.
Comment #41
sunThis one should fix the failing tests.
Comment #42
Dave ReidThe $form['actions'] keeps getting collapsed onto one line when it shouldn't. It's an array that exceeds 80 characters, so it should be wrapped properly.
Here
Here as well.
Powered by Dreditor.
Comment #43
sun@Dave: See #18 for the reasoning.
Comment #44
Dave ReidI see the reasoning but it still doesn't make sense why it needs to violate a coding standard. We don't gain anything by having it on one line.
Comment #45
sun@Dave: AFAIK, there is no such coding standard. The only remotely related is "wrap at 80 chars", but that doesn't necessarily apply to form arrays. I think the one-line notion makes sense in this case, especially provided the reasoning above.
Testbot seems to be stuck in the retest request, so I'm re-attaching the last patch.
Comment #46
Dave ReidSorry sun. I hate to fight about a minor detail of an important patch, but standards matter to me. :/
http://drupal.org/coding-standards#array:
Comment #47
RobLoachI've noticed that as an unwanted side effect, this makes it difficult to inject new stuff above the actions.... Check this out:
Hmmm, maybe we need a consistent #weight that's applied to the action buttons?
Comment #48
sunI agree. Let's come back to the previous patch after ruling this out.
Did I forget anything in here?
Comment #49
sunRTBC?
Comment #50
sunoh YAY! This revealed a quite critical bug in form API! :)
Comment #51
chx CreditAttribution: chx commentedPrior to handling, to is missing.
And http://webchick.net/patch-reviewers-are-not-clairvoyant
Comment #52
jide CreditAttribution: jide commented+1 for a high default weight, see #35.
Comment #53
jide CreditAttribution: jide commented@chx : something missing in your comment ?
Comment #54
sun#50: drupal.form-actions.50.patch queued for re-testing.
Comment #55
Danny_Joris CreditAttribution: Danny_Joris commentedGreat idea guys. I'm looking for a way to make a wrapper in D6. I'm using image replacement for buttons quite a lot using this technique: http://drupal.org/node/692782#comment-2508628 . It works great, except is difficult to theme the button positions when their positions have to be set to 'absolute'. This is why I need a wrapper around buttons.
Thumbs up!
Comment #56
sun@Danny_Joris: http://drupal.org/project/button_style
Comment #57
sun- Fixed the grammar in that inline comment.
- Fixed previously missing/new occurrences of $form['submit']. Entirely left out test modules though.
Comment #59
sunFixed that failing test.
Comment #60
RobLoachChecked a bunch of forms and the wrapper is there. Setting this to RTBC.
Comment #61
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD.
Comment #63
mlncn CreditAttribution: mlncn commentedCouldn't find the moving the node submit buttons to the actions array committed by Dries (mentioned by webchick in #28), but i think documentation can be covered here.
Added the below to Converting 6.x modules to 7.x.
Removing Needs Documentation tag but please review this documentation!