Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I have troubles theming the form or better to say, it's not themed like every other core form. Please fix it so it's themed like every other form in the system. There are more issues than only the label.
Examples:
theme_form_element()
is not used. Custom and standard core classes are missing.theme_form_element_label()
is not used. Custom and standard core classes are missing.
Views exposed form:
<form class="mycustom-form-class" action="foo" method="get" id="views-exposed-form-foo" accept-charset="UTF-8">
<div>
<div class="views-exposed-form">
<div class="views-exposed-widgets clearfix">
<div id="edit-postal-code-wrapper" class="views-exposed-widget views-widget-filter-field_address_postal_code">
<label for="edit-postal-code">Zip code</label>
<div class="views-widget">
<div class="form-item form-type-textfield form-item-postal-code mycustom-text-class">
<input type="text" id="edit-postal-code" name="postal_code" value="" size="30" maxlength="128" class="form-text">
</div>
</div>
</div>
<div class="views-exposed-widget views-submit-button">
<input class="mycustom-button-class form-submit" type="submit" id="edit-submit-foo" name="" value="Filter">
</div>
</div>
</div>
</div>
</form>
How it should looks like (normal form code):
<form class="contact-form mycustom-form-class" action="/user/1/contact" method="post" id="contact-personal-form" accept-charset="UTF-8">
<div>
<div class="form-item form-type-textfield form-item-name mycustom-text-class">
<label class="mycustom-form-label-class" for="edit-name">Your name</label>
<input type="text" id="edit-name" name="name" value="Foo" size="60" maxlength="255" class="form-text">
</div>
...
Labels are normally inside the same DIV like the input. This causes several style issues that can be avoided. Aside of this my custom form theming takes no action as core form API functions are not used, too.
Comment | File | Size | Author |
---|---|---|---|
#59 | vdc-1812048-59.patch | 7.04 KB | tim.plunkett |
#57 | 1812048-57-exposed_form_fapi.patch | 7 KB | mikeker |
#57 | 1812048-57-interdiff.txt | 1002 bytes | mikeker |
#54 | 1812048-54-exposed_form_fapi.patch | 7.29 KB | mikeker |
#47 | interdiff.txt | 1.34 KB | dawehner |
Comments
Comment #1
hass CreditAttribution: hass commentedComment #2
hass CreditAttribution: hass commentedComment #3
hass CreditAttribution: hass commentedviews-exposed-form.tpl.php
is not like coreComment #4
dawehnerWe will not change the output of exposed forms in d7, that's like jumping in cage full of lions.
Comment #5
xjmBugs are things that don't work.
Comment #6
mikeker CreditAttribution: mikeker commentedHere's a first stab at undoing Views' custom theming of exposed filters.
Note: One thing that Views used to do was collect all the single checkbox filters and put the into one widget. The patch removes that as core doesn't do that elsewhere that I'm aware of. And now that exposed forms are using FAPI, the same could be accomplished by hook_form_alter().
Comment #8
mikeker CreditAttribution: mikeker commentedPatch was hit with a Twig... Reroll coming soon.
Comment #9
mikeker CreditAttribution: mikeker commentedRerolled and Twig-ified.
Comment #10
mikeker CreditAttribution: mikeker commentedKnew I forgot to do something...
Comment #11
dawehnerJust a short review.
We should use a library for that.
Comment #12
mikeker CreditAttribution: mikeker commentedThanks for the review dawehner.
I was following the pattern in core\modules\views\lib\Drupal\views\ViewExecutable.php @445 where views.base.css is added. I figured adding the CSS only for views with exposed forms would reduce the CSS loaded for other Views-generated pages.
There's really not much in the new CSS file -- it could probably be combined with the existing views.base.css -- but it may grow.
Comment #13
dawehnerWell, never trust existing code :) There is no reason to not put stuff into a library and use it like that.
Comment #14
mikeker CreditAttribution: mikeker commentedHa! Fair point...
OK, I've added #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] to make the change from #attached to drupal_add_library(). Figured since that was outside the scope of this issue, it warranted a new issue. If we can commit this patch with the #attached call, then the patch in #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] will apply and can be reviewed and (hopefully) committed.
Comment #15
PanchoRerolled after views.base.css is now views.module.css.
Also rolling in the switch to using views.exposed_form.css as a library which was removed between #2005616-10: Views should use ['#attached']['library'] rather than ['#attached']['css'] and #2005616-13: Views should use ['#attached']['library'] rather than ['#attached']['css'].
Comment #17
Pancho"The test did not complete due to a fatal error," but why should LocaleTranslationTest fail on this patch?
#15: exposed-form-fapi-1812048-15.patch queued for re-testing.
Comment #19
Pancho"Setup environment: failed to clear checkout directory," so one more time retesting.
#15: exposed-form-fapi-1812048-15.patch queued for re-testing.
Comment #20
mikeker CreditAttribution: mikeker commented@Pancho, Thanks for the rerolls.
FYI: the views.exposed-form library bit was removed from #2005616: Views should use ['#attached']['library'] rather than ['#attached']['css'] so that it would apply and pass the testbot. Now that that issue is committed, we can add views.exposed-form as a library (as you did in the reroll) and everyone (including the testbot) is happy.
Comment #21
PanchoI know - that's why I rolled it in... :)
The testbot is quite erratic these days, but as long as it doesn't produce false positives, everything's fine...
Comment #22
dawehnerWhat should we do with the chexkbox logic?
Comment #23
mikeker CreditAttribution: mikeker commentedAre there any other places in core that combine several checkboxes into a single form element? I'm not aware of any, which makes me think removing the old checkbox logic makes this more consistent.
Also, now that we're using FAPI, couldn't that be done with hook_form_alter if needed for a given project?
Comment #24
mikeker CreditAttribution: mikeker commentedA few more tweaks:
Comment #25
hass CreditAttribution: hass commented"LRT" typo... and what about the rtl file? :-)
Comment #26
mikeker CreditAttribution: mikeker commentedThanks for the feedback. LRT... Gotta love copy/paste errors!
I've added the rtl file and added some PHPDoc @file comments to be the beginning of both.
Comment #27
mikeker CreditAttribution: mikeker commentedAdd the patch...
Comment #28
hass CreditAttribution: hass commentedI'm not 100% sure, but I think you need to set
margin-right
in RTL to 0 for.views-exposed-form .views-exposed-widgets
.Comment #29
tim.plunkettWe now have Views in core by default with exposed filters, on admin/content and admin/people.
Can we see screenshots of them with this patch?
For even more real-world testing, screenshots with #2020167: Add a name and email search field to the admin/people view and #2001922: Improve admin/content view would be even better.
Comment #30
mikeker CreditAttribution: mikeker commented@#28: the
margin-left: 0
is correct. It's setting the margin between the exposed form widgets, which are floated right, and the Apply/Submit button.@#29: Will do.
Comment #31
hass CreditAttribution: hass commentedLeft is correct, but right may be wrong as is not reset.
Comment #32
mikeker CreditAttribution: mikeker commentedThat's my quote of the month! :)
Now I understand your point -- I'll investigate what the default is for rtl displays and get back to you. But I have zero experience in rtl so if someone else looking at this issue knows the answer, please chime in!
Comment #33
mikeker CreditAttribution: mikeker commentedOK, some screenshots. Finally...
Latest patch in the this issue as well as #2020167-19: Add a name and email search field to the admin/people view
Compared to #2020167-19: Add a name and email search field to the admin/people view only
Compared to D7
Comment #34
hass CreditAttribution: hass commentedTry this in RTL file, please.
Comment #35
mikeker CreditAttribution: mikeker commentedAs suggested by @hass in @#34. Thanks, hass!
Comment #36
dawehnerIsn't that issue about moving exposed forms to FAPI and nothing else? I actually don't think that we want to have a different styling for every exposed form.
Comment #37
hass CreditAttribution: hass commentedHe only kept the current style as I know. We should change style later if required. The main reason for opening the case was that I need the same html structure like core and to make sure css classes injected via preprocess are passed to views forms, too. Otherwise my forms are not styled the same way everywhere on the site.
Comment #38
dawehnerYes hass, that is what I thought as well. But doesn't #1812048-33: Build the exposed form using form API functions changes the styles of exposed forms?
Comment #39
mikeker CreditAttribution: mikeker commented#38: Yes, it does. I changed the styling to make exposed forms act more like FAPI forms (see the D7 vs. D8-with-patch screenshots in #33).
Though you're probably right -- that would make the scope of this issue much larger as it'll be affecting all the newly ported-to-Views admin pages. Yikes...
I'll reroll without the styling changes and get some before/after screenshots together.
Comment #40
mikeker CreditAttribution: mikeker commentedOK, reroll keeping the styling as similar as possible. @hass, please check the RTL file to make sure I've done that correctly. Thanks!
Attached are before/after screenshots of the admin/people and admin/content pages.
For whatever reason I'm unable to embed those shots in this comment...But I can include img tags! :)admin/people before:
admin/people after:
admin/content before:
admin/content after:
Comment #42
tim.plunkett#40: 1812048-40-exposed_form_fapi.patch queued for re-testing.
Comment #43
dawehnerLet's not change the name of the label ... as this would affect people when upgrading etc.
Comment #44
mikeker CreditAttribution: mikeker commentedI was wondering about that... However on the pages with VBO fields it would lead to two "Apply" buttons. Also the D7 version of those pages used "Filter". Finally the text can be overridden in the exposed form settings.
Thoughts?
Comment #45
dawehnerIt is out of scope of this issue sorry, so having this not in will help to get it in.
Comment #46
mikeker CreditAttribution: mikeker commentedOK.
Comment #47
dawehnerEven that is out of scope, sorry. Actions might make sense in admin forms, but do they make sense on the normal page? Let's discuss that in a follow up.
This also brings back the checkbox handling.
Comment #49
dawehner#47: vdc-1812048-47.patch queued for re-testing.
Comment #50
mikeker CreditAttribution: mikeker commented#47: I'm happy to push that into a followup issue, but I should point out that it's the same form code as the VBO "Apply" button which makes the buttons style similarly (cleared left, flush to the left margin). Otherwise it'll be floated left beside (to the right of) the other exposed form elements.
I suppose the larger question is: are we aiming for consistency with D7's exposed forms or D8's FAPI forms? I guess I was aiming to make D8 exposed forms look like other D8 FAPI forms, but I can see the reasoning for keeping them more like D7 forms.
I can't find any single checkbox exposed form elements. Am I missing them, or is that code leftover an earlier era when Views did checkboxes? If this is old code, can we leave it out?
Comment #51
dawehnerDuring the time I had several use cases for that, but well we could just drop it now and people should just put some theming if they really want to.
Actions seems to be interconnected with normal edit forms.
Comment #52
mikeker CreditAttribution: mikeker commentedre: single checkbox handling: my opinion is that we should limit the custom form editing in Views as much as possible -- that keeps exposed form consistent with other FAPI forms.
If we don't, I promise a bunch of "why doesn't this work like other forms?" bugs being filed in the year after D8 releases! :)
Gotta dash -- will come back to comment on actions later...
Comment #53
dawehnerYou convinced me 100%! Go with actions and remove the custom checkbox handling.
Comment #54
mikeker CreditAttribution: mikeker commentedApologies for the delay -- I had too many relatives in town for the holidays and am just now recovering...
Glad I could convince you! :)
Attached is basically #46 against the most recent 8.x head. Removing "Needs screenshots" as those are in #40 with the only difference being the text on the button changing from Filter to Apply.
Comment #55
dawehnerThis is not a problem, though that
As far as I know there are new css styles, which does RTL and normal styling in the same file.
Comment #56
hass CreditAttribution: hass commentedJepp, https://drupal.org/node/2032405
Comment #57
mikeker CreditAttribution: mikeker commentedI'm learning something new every day in this issue! :)
Attached patch rolls the RTL into the exposed form CSS as per #56. Thanks for the link, hass.
Comment #58
dawehnerThank you very much.
Comment #59
tim.plunkettAll of the $vars are now $variables.
Blocks #933004: Test that all form elements have a title for accessibility
Comment #60
alexpottManually tested all looking good...
Committed 535ed41 and pushed to 8.x. Thanks!