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.
For theming, it would be beneficial to have a .form-label-none
class on form items that have no or no visible <label>
, so the margin and padding can be adjusted accordingly.
This gets especially important when theming forms in a special way, or when theming forms in a two-column layout (i.e., label left, input right).
Comments
Comment #1
andypostGood idea
Comment #2
patrickd CreditAttribution: patrickd commentedThis way?
Comment #4
gnugeti just re-attach a new version of the patch with a fix
Comment #5
patrickd CreditAttribution: patrickd commented(omg, how could that happen to me x,D)
patch looks good for me :)
Comment #6
andypostThat's great but we need a test for this
Comment #7
patrickd CreditAttribution: patrickd commentedgeneral test of functionality or simpletest testcase?
Comment #8
sun#title_display can also be 'attribute' and something else (see code lines a bit below this line).
We need to check explicitly for all of them, either the positive ones or the negative ones. I guess that checking for 'before' or 'after' might be the shortest condition.
That said, yes, this needs tests. The test needs to check #title (unset vs. set) and (all) #title_display values.
Comment #9
gnugetOks i changed the condition for check the before and after value on the title_display .
I attached another patch.
I'm going to provide a tests in my next comment.
Comment #10
gnugeti created a set of tests for this issue is my first time making tests so i'm sure to this could be done in a better way. I would really appreciate if someone else with more experience can check my tests
Thanks!
Comment #11
kathyh CreditAttribution: kathyh commentedchanged to needs review for a quick testbot check
Comment #12
tstoecklerThis looks quite good. Some minor issues with the comments though:
Also below:
css -> CSS
Could this extend DrupalUnitTestCase instead? It seems like it could...
I think we could shorten that by doing:
Also comments should be prepended with a space and be Sentence case, i.e.
"// None" instead of "//none"
css -> CSS
css -> CSS
Comment #13
sunNope. Every test case that relies on hooks or the theme registry (drupal_render()) needs to be a DrupalWebTestCase.
However, this test class can use
protected $profile = 'testing';
to speed up the test case setup.--
Also:
- You may want to assign the returned $output of drupal_render() to $this->drupalSetContent(), and then simply use assertRaw() to check whether the CSS class exists. There's also an assertPattern() helper method for PCREs, but not sure whether that is really necessary here.
- The test should also confirm at least one counter-case; i.e., a condition in which the CSS class does not appear. That is, because it needs to confirm that the condition is not mistakenly triggered always. In fact, every test case that attempts to verify conditional behavior is supposed to do that (but we still have plenty of tests setting a wrong example throughout core).
Comment #14
gnugetThanks for all your feedback.
I attach a new and improved version of the patch.
In my first tests i added two cases where the CSS class form-label-none not suppose to appear (the case for 'after' and 'before') this time i added another case where the user add a non expected value for the #title_display option, thank you.
Comment #15
scresante CreditAttribution: scresante commented#14: form_no_label_class_with_tests-1419036-14.patch queued for re-testing.
Comment #17
scresante CreditAttribution: scresante commentedThe directory structure of the tests has changed drastically.
The second half of the above patch will have to be implemented somewhere within core/modules/system/lib/Drupal/system/Tests/Form/ probably in ElementsLabelsTest.php
Comment #18
scresante CreditAttribution: scresante commentedThis is a correction of the first part of the patch and should work cleanly.
The tests still need to be re-written and I'm not clear on how to do that.
Comment #19
scresante CreditAttribution: scresante commentedComment #20
LinL CreditAttribution: LinL commentedSetting to "needs work" for the tests.
Also unassigning, hope that's OK, gnuget?
Comment #21
oliverhm CreditAttribution: oliverhm commentedComment #22
oliverhm CreditAttribution: oliverhm commentedI've created a form and tested the following test scenarios:
- elements with the #title property missing
- elements with the #title property empty
- elements with the #title_display set to hidden
In order to work on all these scenarios I had to change the previous patch, the new patch is attached.
Comment #24
Mschudders CreditAttribution: Mschudders commented22: 0001-fixing-issue-1419036.patch queued for re-testing.
Comment #25
gnugetHi Oliver.
Please check #8 we need to check explicitly for all the possible values in the title_display option.
#18 works but the tests still need to be re-written, so this needs work
Comment #26
InternetDevels CreditAttribution: InternetDevels commentedNew patch attached.
Comment #27
sunThanks!
Drupal\system\Tests\Form\FormTest
..form-no-label
if (!isset($element['#title']))
condition a few lines up. After doing so, the necessary condition is simply:Comment #28
wheatpenny CreditAttribution: wheatpenny commentedI working on this issue per the post DrupalConNA sprint weekend.
Comment #29
wheatpenny CreditAttribution: wheatpenny commented26: drupal_core-add_a_css_class_for_form_items_that_have_no_label-1419036-26.patch queued for re-testing.
Comment #31
wheatpenny CreditAttribution: wheatpenny commentedAttached patch addresses points 2 and 3 from #27.
Comment #32
wheatpenny CreditAttribution: wheatpenny commentedComment #34
wheatpenny CreditAttribution: wheatpenny commentedComment #35
lauriiiThis should fix the tests
Comment #37
lauriiiThere was still cases that were not covered such as invisible labels. I also applied tests to this patch.
Comment #39
wheatpenny CreditAttribution: wheatpenny commented37: class_to_form_items_without_label-1419036-37.patch queued for re-testing.
Comment #40
wheatpenny CreditAttribution: wheatpenny commentedI applied the passing patch from #37. According to my testing, the class was added for a field that has no label.
Field with label (no added class):
Field with no label (added "form-no-label" class):
I'm leaving this issue as "Needs review" as I am not familiar enough with testing to comment on the tests in the patch. The tests still need to be reviewed.
Comment #41
lauriiiOne line is being deleted in the patch by a mistake. I'll fix that.
Comment #42
lauriiiRemoved unnecessary line removal from the patch.
Comment #43
lauriiiComment #44
Sir-Arturio CreditAttribution: Sir-Arturio commentedThe patch still adds the form-no-label. Tested both manually (see the attached images) and automatically (with just the test [failed] and test+fix [success]).
Comment #45
alexpottCommitted 78a7670 and pushed to 8.x. Thanks!