Comments

mherchel created an issue. See original summary.

djsagar’s picture

Assigned: Unassigned » djsagar
djsagar’s picture

Assigned: djsagar » Unassigned
StatusFileSize
new1.73 KB
new78.44 KB
new74.92 KB

Hi @mherchel,

I create patch for Views exposed filters to making inline.

Please review.

Thanks!

djsagar’s picture

Status: Active » Needs review
mherchel’s picture

StatusFileSize
new1.46 KB
new2.45 KB

I like the idea of adding specific .form--inline styles. I wasn't initially considering it, but it makes sense.

I'm attaching an updated patch that adds that class directly onto the Views exposed filters form, and I'm also using display: inline-block to handle the inline elements instead of float. This will handle RTL a bit better, and is more what we're trying to do.

cindytwilliams’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new20.78 KB
new15.73 KB

Path #5 applies cleanly, and displays the Views exposed filters inline using the form--inline class. Marking RTBC.

Before:
before patch

After:
after patch

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3224958-5.patch, failed testing. View results

mherchel’s picture

Status: Needs work » Reviewed & tested by the community

Last test failure is unrelated.

Field_layout.Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest
✗	
Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest
exception: [Other] Line 0 of sites/default/files/simpletest/phpunit-47.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.8 by Sebastian Bergmann and contributors.

Warning:       Your XML configuration validates against a deprecated schema.
Suggestion:    Migrate your XML configuration using "--migrate-configuration"!

Testing Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest
....E                                                               5 / 5 (100%)

Time: 01:23.653, Memory: 4.00 MB

There was 1 error:

1) Drupal\Tests\field_layout\FunctionalJavascript\FieldLayoutTest::testChangingFormatterAndRegion
WebDriver\Exception\CurlExec: Curl error thrown for http POST to http://chromedriver-jenkins-drupal-patches-94345:9515/session/b121eb8e4dff9e092b2581b532c68634/execute with params: {"script":"(function (element) {\n    var event = document.createEvent(\"HTMLEvents\");\n\n    event.initEvent(\"drop\", true, true);\n    event.dataTransfer = {};\n\n    element.dispatchEvent(event);\n}(arguments[0]));","args":[{"ELEMENT":"0.22933148352516053-2"}]}
lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new15.41 KB

How do we expect this to look when the form element has a description?

mherchel’s picture

Status: Needs review » Needs work

How do we expect this to look when the form element has a description?

Not like that!

mherchel’s picture

Status: Needs work » Needs review
StatusFileSize
new1.74 KB
new1.11 KB
new53.85 KB

Fix attached!

andy-blum’s picture

Status: Needs review » Reviewed & tested by the community

This all looks good! Moving to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3224958-11.patch, failed testing. View results

mherchel’s picture

Status: Needs work » Reviewed & tested by the community

Test failure is unrelated.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3224958-11.patch, failed testing. View results

mherchel’s picture

Status: Needs work » Reviewed & tested by the community

Another unrelated Layout Builder test failure:

Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-84.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.8 by Sebastian Bergmann and contributors.
mherchel’s picture

mherchel’s picture

  • lauriii committed 8c35867 on 9.3.x
    Issue #3224958 by mherchel, djsagar, cindytwilliams, lauriii: Olivero...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Tested this manually in various use cases and it worked as expected!

Something that caught my attention was that form--inline isn't implementing BEM properly. However, it seems to be a pre-existing issue. Discussed with @mherchel and we agreed that this is something that should be handled globally, for example in #2417111: Replace container-inline with form--inline to display forms horizontally..

Our pre-existing use cases are using .form--inline .form-item as a selector. I like the approach here because I think it's fair to assume that inside .form--inline all elements are inline, regardless of whether they have .form-item class or not. The approach on this patch also seems potentially more compatible with BEM because the approach currently taken by core, adds a dependency between block level elements. Discussed with @mherchel and he said he'd open a follow-up for refactoring other themes to use this pattern.

Committed 8c35867 and pushed to 9.3.x. Thanks!

Didn't backport to 9.2.x because of the new hook implementation.

Status: Fixed » Closed (fixed)

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