Layout builder filter leaves blank space

Thanks for reporting and the work! Can confirm the patch resolves the issue in latest 9.2.x.

Fixed custom commands failure

Patch at #5 also works for me on Drupal core 9.2.4

Agreed, but it is missing tests. It should be possible to assert that only the expected <li> tags are visible.

Working on a test

Here's a test. Turns out there was an existing one that made the same bad assumption the JS made!

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -46,6 +46,7 @@
              $link.toggle(textMatch);
   +          $link.parent().toggle(textMatch);
   

   This shouldn't need to be a second call. Instead, replace the existing toggle call with the parent call. That will toggle them both.

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -84,6 +85,7 @@
              $filterLinks.show();
   +          $('.js-layout-builder-block-link').parent().show();
   

   This selector is the same as $filterLinks. So similar to the last piece of feedback, both of these links could be replaced with $filterLinks.parent().show();

@smustgrave Thanks! It would help to know: is that true with the latest MR and with the patch from #10? I know that part works fine without the patch...
That will help understand if my suggestions after #10 were what broke things, and then the tests can be expanded appropriately.

If you could test https://www.drupal.org/files/issues/2021-10-07/3223048-filter-10-PASS.patch from #10 as well that'd be great

Verified and tested patch #24.
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Seven theme
# Goto: Extend -> Install Layout Builder Module
# Goto: /admin/structure/types/manage/page/form-display
# Enable the layout setting:
- Use Layout Builder
- Allow each content item to have its layout customized.
# Save it
# Create Basic page content type
# Save it and Goto: Layout option
# Click on Add Block
# Search any keyword like "Au"

Expected Results:
# After applying patch #24, User should not see blank space while searching any keyword

Actual Results:
# Currently, the User is able to see blank space while searching any keyword

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

This is missing test coverage for all changes since the MR in #14

Applied #24 patch applied successfully and showing proper result without any space
Thanks for the patch

Added reroll of patch #24 on Drupal 9.4.x

The MR in #14 passed tests, but was broken. That means the test coverage is insufficient.
The new test coverage should fail with the patch from #14, but pass with the patch from #24/#31.

Resolved CS issues.

Moving to NW for tests.

Fixed the below exception :

Drupal\Tests\layout_builder\FunctionalJavascript\BlockFilterTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-92.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.19 �[44m#StandWith�[0m�[43mUkraine�[0m

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

Attaching the updated patch

I am using this CSS and the blank lines with bullets no longer appear.

.links .js-layout-builder-block-link {
  color: inherit;
  margin: auto 1rem;
}

.links .js-layout-builder-block-link:hover {
  text-decoration: underline;
}

.js-layout-builder-categories ul.links {
  margin: 0;
  padding: 0;
  list-style: none;
}

.js-layout-builder-categories .claro-details__wrapper.details-wrapper {
  margin: 0 0.5rem 0.5rem 1.4rem;
}

.js-layout-builder-categories ul.links li {
  padding: 0;
  margin: 0;
}

.js-layout-builder-categories .links .js-layout-builder-block-link {
  display: block;
  margin-bottom: 0.25rem;
}

Tested latest patch; passed tests

As I said in #32, no tests have been added since #14. Also why was the MR abandoned for patches? It makes it extremely hard to follow the changes being made.

Here's the interdiff. I believe I was specifically referring to your comment in #17, which isn't reflected in the test coverage as far as I can see.

It's also not clear why we're toggling two elements each time now.

I have successfully applied the last patch for 9.5.x. Adding Screenshots for the reference.

Verified and tested the patch #60. Patch applied successfully.

Test Steps:
1. Enable 'Seven Theme' and install 'Layout Builder Module'
2. Goto /admin/structure/types/manage/page/form-display
3. Enable the layout setting:
- Use Layout Builder
- Allow each content item to have its layout customized.
4. Create 'Basic page' content type
5. Goto 'Layout' option
6. Click on 'Add Block'
7. Search any keyword like "Au", "cod", "user"

Test Result: No blank space is rendered on searching keywords
Attached screenshot for reference. RTBC +1

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -45,7 +45,10 @@
   +          if ($(link).closest('.js-layout-builder-category').is(':hidden')) {
   +            $(link).closest('.js-layout-builder-category').show();
   +          }
   

   We have $(link) multiple times here. Probably worth a local variable? Also this deserves a comment explaining why we're show()ing something right before toggling

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -84,13 +87,14 @@
              $filterLinks.show();
   +          $filterLinks.parent().show();
   

   The first double toggle was removed, but now we have this one too. This was added back since #14, and I don't know when because someone abandoned the MR for patches again...

@smustgrave, the MR looks good & in the recent version I can also see that all the points mentioned by @tim.plunkett at #64 is covered. I've fixed the trivial reports by CodeSniffer to the MR.

NB: The MR is still pointed to 9.3.x; shouldn't the target branch be 9.5.x?

I'm also attaching the backported patch for 9.4.x.

I had have the same issue on Drupal 9.4.x
I modified few things and applied this patch and it worked.

We need a version of the patch in #69 that applies to 10.1.x & 10.0.x. Thanks and sorry as they might not have necessary back in July :(

Rerolled patch #72 for drupal 10.0.x.

Comparing #69 and #72 we have lost the comments:

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -43,9 +43,15 @@
+          // Checks if a category is currently hidden. Toggles the category on if so.
...
+          // Toggle the li tag of the matching link.

@@ -82,15 +88,17 @@
+          // Show all categories since filter is turned off.
...
+          // Show all li tags since filter is turned off.

I think these should remain in the 10.x version of the patch.

Hi, Is this issue fixed in 10.1.x? I couldn't find the issue in 10.1.x version. It is working fine without the patch

Only one minor observation here, leaving at needs review

+++ b/core/modules/layout_builder/js/layout-builder.js
@@ -43,9 +43,15 @@
+          const linkJquery = $(link);
...
+          if (linkJquery.closest('.js-layout-builder-category').is(':hidden')) {
...
+          linkJquery.parent().toggle(textMatch);

I think our convention is to name variables that are a jQuery instance with a leading $

so perhaps $link instead of linkJquery

We're actively trying to remove jQuery from the project, so are we sure we want to add in new instances here? Is there a way we can achieve this with Vanilla JS.

edit: - nope we'd need #3167377: Rewrite jQuery .show() and .hide() for that, so anyone following this issue, please lend a hand with some reviews over there if you can 💙

Thanks for the patch, @smustgrave. As mentioned by @longwave:

We're actively trying to remove jQuery from the project, so are we sure we want to add in new instances here? Is there a way we can achieve this with Vanilla JS.

I've tried to convert the existing jQuery changes as per Vanilla JS.

Hiding #85 as my comment said, no we don't need to do that here as we don't have a solution for show/hide

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

+++ b/core/modules/layout_builder/js/layout-builder.js
@@ -43,9 +43,17 @@
+          $linkJquery.parent().toggle(textMatch);

nitpick/personal preference, I'd name this $link - the $ indicates its a jQuery object

I think we need a 9.5 version of this patch with the es6 and compiled version

Applied patch #91 on Drupal 10.1.x-dev.
The patch applied cleanly.
The issue got fixed after applying the patch.
Refer to the previous before and after screenshots.

Also, tried to apply the patch on the 9.5 version but patch failed to apply with below error:

Checking patch core/modules/layout_builder/js/layout-builder.js...
error: while searching for:
         *   The link to add the block.
         */
        const toggleBlockEntry = (index, link) => {
          const textMatch =
            link.textContent.toLowerCase().indexOf(query) !== -1;
          $(link).toggle(textMatch);
        };

        // Filter if the length of the query is at least 2 characters.

error: patch failed: core/modules/layout_builder/js/layout-builder.js:43
error: core/modules/layout_builder/js/layout-builder.js: patch does not apply
Checking patch core/modules/layout_builder/tests/src/FunctionalJavascript/BlockFilterTest.php

Verified and tested patch #91 on 10.1.x-dev. Patch applied cleanly.

Test Results:
1. Blank Space issue is no longer reproducible on 10.1.x-dev.
2. When changing a matching string to a shorter matching string then filter work and results match. For ex. When searched for "ch", then searched "cha" and then again searched "ch", all matching results are displayed.

Refer attached video for before and after patch.

@smustgrave This patch resolves the https://www.drupal.org/project/drupal/issues/3103506 issue.

Here is a re-roll for the 9.5.3 version

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

+++ b/core/modules/layout_builder/js/layout-builder.js
@@ -43,9 +43,15 @@
+          // Checks if a category is currently hidden. Toggles the category on if so.

extreme nit pick ™️ - greater than 80 chars here

other than that I think this is ready

Added screenshots to issue summary

Committed a5291f0 and pushed to 10.1.x. Also cherry-picked to 10.0.x. Thanks!

We still need a patch for 9.5.x since this is eligible for backport.

Adding patch for 9.5.x as requested in #108

Solving CCF from#110

Solving CCF from#112

Re-rolled patch for 9.5.x, attached interdiff for same.

Committed 06eac4f and pushed to 9.5.x. Thanks!