Problem/Motivation

Claro theme does not have the right form error class.
The files core/themes/claro/templates/details.html.twig and core/themes/claro/templates/navigation/details--vertical-tabs.html.twig have the class "form-item--error-message" but in the stylesheet core/themes/claro/css/components/form.css is defined "form-item__error-message" so if any error is displayed on the details component it is shown without styles.

Steps to reproduce

  • Make claro the default theme, so the test page we visit is rendered by Claro
  • Enable the inline_form_errors module
  • Enable the form_test module. It is a test module so you'll need $settings['extension_discovery_scan_tests'] = TRUE; in your settings if it isn't already there.
  • Go to form_test/details-form and submit the form. There will be an error "I am an error on the details element." that should be styled red but it is not due to the class being incorrect as reported

Proposed resolution

Use the right class.

Test for this by expanding \Drupal\form_test\Form\FormTestDetailsForm to try with Claro in addition to the default test theme, and assert the expected class is present.

CommentFileSizeAuthor
#13 1.jpg74.93 KBskipper-vp
#8 3298580-8.patch1.14 KBakram khan
screenshot_failed_class_claro.jpg57.49 KBeduardo morales alberti

Issue fork drupal-3298580

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Eduardo Morales Alberti created an issue. See original summary.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Version: 9.4.x-dev » 9.3.x-dev

Eduardo Morales Alberti opened merge request !2526

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Active » Needs review
Bushra Shaikh’s picture

Bushra Shaikh commented
Assigned: Unassigned » Bushra Shaikh
Bushra Shaikh’s picture

Bushra Shaikh commented
Assigned: Bushra Shaikh » Unassigned
smustgrave’s picture

smustgrave commented
Version: 9.3.x-dev » 10.1.x-dev
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

MR should be updated for 10.1

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 3298580-8.patch1.14 KB

Updated Patch for drupal 10.1.x

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Think this could also use a test case showing the issue.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
skipper-vp’s picture

skipper-vp commented
StatusFileSize
new 1.jpg74.93 KB

I have tried to look into the issue manually but couldn't find the error you're describing.
I can see the error in the message area, but not on the element itself, moreover dumping errors variable from details.twig returns null (see screenshot). Could you give me a hint if I'm missing out on anything?

Missing error

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue summary: View changes
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Re #13 enabling the Inline Form Errors module should get this to show up I just updated the issue summary to include this in steps to reproduce.

Utkarsh_33 made their first commit to this issue’s fork.

Utkarsh_33 opened merge request !8037

utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review

Test for this by expanding \Drupal\form_test\Form\FormTestDetailsForm to try with Claro in addition to the default test theme, and assert the expected class is present.

We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs review » Needs work

We are making changes in claro templates only.Do we need to do the same of default theme and then expand tests or shall we create a follow-up issue for default theme?

\Drupal\form_test\Form\FormTestDetailsForm already tests the default theme. it is simplest to add a few lines to that test to run the test with the default theme and then with Claro. Use a @dataProvider

Set up your dataProvider and put this at the beginning of the test to set the theme when $theme isn't false, and that does everything needed without having to add a new test class that is 99% the same as an existing one.

if ($theme) {
$this->config('system.theme')
      ->set('default', $theme)
      ->save();
}
utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review

smustgrave changed the visibility of the branch 3298580-claro-details-component to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Added some nitpicky typehints

The failure is unrelated to this change.

  Drupal\FunctionalTests\Installer\InstallerTranslationExistingFileTest::testInstall
    Behat\Mink\Exception\ExpectationException: Current response status code is
    403, but 200 expected.

Believe this one is good.

nod_ closed merge request !8037

  • nod_ committed 4f0a7ce2 on 10.3.x 
    Issue #3298580 by Utkarsh_33, Eduardo Morales Alberti, smustgrave, Akram...

  • nod_ committed 0dd26ceb on 10.4.x 
    Issue #3298580 by Utkarsh_33, Eduardo Morales Alberti, smustgrave, Akram...

  • nod_ committed 0c542167 on 11.0.x 
    Issue #3298580 by Utkarsh_33, Eduardo Morales Alberti, smustgrave, Akram...

  • nod_ committed 8f25e5c2 on 11.x 
    Issue #3298580 by Utkarsh_33, Eduardo Morales Alberti, smustgrave, Akram...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for the fix and thanks for including a test, in this case the fix doesn't warrant a test and I removed it on commit.

Committed and pushed 8f25e5c227 to 11.x and 0c542167d6 to 11.0.x and 0dd26ceb53 to 10.4.x and 4f0a7ce2c8 to 10.3.x. Thanks!

nod_ credited longwave.

nod_ credited quietone.

nod_’s picture

nod_
Primary language French
Location Lille
commented

credit to longwave and quietone for helping with testing decision

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.