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.
Problem/Motivation
The filtering works fine but when links are hidden the li tags have padding and/or border that stays behind thus taking up space.
Steps to reproduce
Proposed resolution
Hide the parent li
Before
After
Remaining tasks
N/A
User interface changes
No empty li tags
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Comment | File | Size | Author |
---|---|---|---|
#115 | interdiff-113_115.txt | 1.67 KB | Gauravvvv |
#115 | 3223048-115.patch | 5.91 KB | Gauravvvv |
#113 | 3223048-113.patch | 4.24 KB | Rishabh Vishwakarma |
#112 | 3223048-112.patch | 4.25 KB | Rishabh Vishwakarma |
#110 | 3223048-110.patch | 4.26 KB | Rishabh Vishwakarma |
Issue fork drupal-3223048
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:
- 3223048-layout-builder-filter changes, plain diff MR !1310
Comments
Comment #2
smustgrave CreditAttribution: smustgrave at Mobomo commentedTried to keep it simple. If the link is being toggled then toggle the parent li.
Comment #3
smustgrave CreditAttribution: smustgrave at Mobomo commentedWrong patch. Forgot the change to the es6 file
Comment #4
ryan.gibson CreditAttribution: ryan.gibson commentedThanks for reporting and the work! Can confirm the patch resolves the issue in latest 9.2.x.
Comment #5
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed custom commands failure
Comment #6
kevinting CreditAttribution: kevinting commentedPatch at #5 also works for me on Drupal core 9.2.4
Comment #7
smustgrave CreditAttribution: smustgrave at Mobomo commentedGoing to say this has been tested by the community
Comment #8
tim.plunkettAgreed, but it is missing tests. It should be possible to assert that only the expected <li> tags are visible.
Comment #9
tim.plunkettWorking on a test
Comment #10
tim.plunkettHere's a test. Turns out there was an existing one that made the same bad assumption the JS made!
Comment #12
tim.plunkettThis shouldn't need to be a second call. Instead, replace the existing toggle call with the parent call. That will toggle them both.
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();
Comment #15
Theresa.GrannumComment #16
Theresa.GrannumComment #17
smustgrave CreditAttribution: smustgrave at Mobomo commentedMay need some tweaking. The filtering seems to work but if you delete some letters from your search it doesn't run.
Comment #18
tim.plunkett@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.
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedI tested https://git.drupalcode.org/project/drupal/-/merge_requests/1310.diff only did you want to test a patch also?
Comment #20
tim.plunkettIf 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
Comment #21
smustgrave CreditAttribution: smustgrave at Mobomo commentedSorry for the delay but patch 10 seems to be closer.
So I did a refresh 9.3.x-dev install
Searched for "au" and got
Search for "aut" and got
Search for "au" again and got
Comment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #23
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis should fix the command failure.
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedSorry for the spam!
Comment #25
chetanbharambe CreditAttribution: chetanbharambe at QED42 for Drupal India Association commentedVerified 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.
Comment #26
tim.plunkettThis is missing test coverage for all changes since the MR in #14
Comment #27
smustgrave CreditAttribution: smustgrave at Mobomo commented@tim.plunkett confused what test coverage is needed that wasn't included?
Comment #29
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedApplied #24 patch applied successfully and showing proper result without any space
Thanks for the patch
Comment #30
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoving back to reviewed as I don’t know what tests are needed
Comment #31
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #24 on Drupal 9.4.x
Comment #32
tim.plunkettThe 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.
Comment #33
yogeshmpawarResolved CS issues.
Comment #34
yogeshmpawarMoving to NW for tests.
Comment #35
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed 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
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded an additional test to make sure items reappear.
I did the interdiff off #33 vs #35 because 35 had phpunit config changes that I don't think should be included.
Comment #38
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #39
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot sure why the 1 layout builder test is failing as that didn't change.
The other2 failures are from another module in core, quickedit.
Comment #40
smustgrave CreditAttribution: smustgrave at Mobomo commentedFound another issue. When using the X button to clear it doesn't reset the list.
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #44
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #45
smustgrave CreditAttribution: smustgrave at Mobomo commentedprobably would help if I included the changes disregard #44 it's the same as #42
Comment #47
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot sure why that test is failing if someone more verse in uniting testing wants to take a look. All the blocks should be back when just "a" is filtered.
And since $this->assertAnnounceContains('All available blocks are listed.'); this passes I'm stumped.
Currently having difficulty getting Functionaljavascript test to run locally in my lando instance.
Comment #48
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #49
ianchan CreditAttribution: ianchan commentedI am using this CSS and the blank lines with bullets no longer appear.
Comment #50
brittanydilts CreditAttribution: brittanydilts at Mobomo commentedTested latest patch; passed tests
Comment #51
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #52
brittanydilts CreditAttribution: brittanydilts at Mobomo commentedComment #53
tim.plunkettAs 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.
Comment #54
smustgrave CreditAttribution: smustgrave at Mobomo commentedwhat additional tests are needed that isn't already included?
Comment #55
smustgrave CreditAttribution: smustgrave at Mobomo commented@tim.plunkett I believe the current test cases cover the changes. The block selector was updated to
'.js-layout-builder-categories li'
Even added a test to make sure all the links reappear. When the field clears to cover the change for
$(once('block-filter-text', 'input.js-layout-builder-filter', context)).on('input', debounce(filterBlockList, 200));
Tried to do an interdiff with the PR from 14 but it won't generate.
Please advise what additional tests are needed.
Comment #56
tim.plunkettHere'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.
Comment #57
smustgrave CreditAttribution: smustgrave at Mobomo commentedI see. Think adding a test case for searching by 3 letters then 2 will cover that. I’ll update
Comment #58
tim.plunkettIt's also not clear why we're toggling two elements each time now.
Comment #59
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded some test cases for reducing the search to verify blocks return.
Also removed one of the toggles and everything still seems to work.
Comment #60
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #61
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Srijan | A Material+ Company for Drupal India Association commentedI have successfully applied the last patch for 9.5.x. Adding Screenshots for the reference.
Comment #62
smustgrave CreditAttribution: smustgrave at Mobomo commentedSo can this be marked as RTBC?
Comment #63
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at QED42 for Drupal India Association commentedVerified 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
Comment #64
tim.plunkettWe have
$(link)
multiple times here. Probably worth a local variable? Also this deserves a comment explaining why we're show()ing something right before togglingThe 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...
Comment #65
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated the MR. But prefer patches because they're easier to maintain. That MR was super behind and took some effort to get up to date.
Comment #66
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #68
Rishi Kulshreshtha@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.
Comment #69
smustgrave CreditAttribution: smustgrave at Mobomo commentedI'm abandoning the PR. I can't edit the PR to point to another branch or close it so don't see the value and most people are skipping over it.
#68 looks to have issues so skipping over that and just pulling the code from the PR into a patch file.
Comment #70
Shifali Baghel CreditAttribution: Shifali Baghel at Srijan | A Material+ Company for Drupal India Association commentedI had have the same issue on Drupal 9.4.x
I modified few things and applied this patch and it worked.
Comment #71
alexpottWe 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 :(
Comment #72
smustgrave CreditAttribution: smustgrave at Mobomo commentedHere's a 10.1 patch
Comment #73
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #74
Ratan Priya CreditAttribution: Ratan Priya at OpenSense Labs commentedRerolled patch #72 for drupal 10.0.x.
Comment #75
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #76
smustgrave CreditAttribution: smustgrave at Mobomo commentedPatch #74 should be ignored.
Comment #77
longwaveComparing #69 and #72 we have lost the comments:
I think these should remain in the 10.x version of the patch.
Comment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #79
Asha Nair CreditAttribution: Asha Nair at Zyxware Technologies commentedHi, 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
Comment #80
Asha Nair CreditAttribution: Asha Nair at Zyxware Technologies commentedComment #81
smustgrave CreditAttribution: smustgrave at Mobomo commentedI believe it appears to be working since one category is displayed. Try just 2 characters.
Comment #82
smustgrave CreditAttribution: smustgrave at Mobomo commentedBumping as this has been super annoying for layout builder
Comment #83
larowlanOnly one minor observation here, leaving at needs review
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 💙
Comment #84
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated variable
Comment #85
Rishi KulshreshthaThanks for the patch, @smustgrave. As mentioned by @longwave:
I've tried to convert the existing jQuery changes as per Vanilla JS.
Comment #86
larowlanHiding #85 as my comment said, no we don't need to do that here as we don't have a solution for show/hide
Comment #87
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #88
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #89
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #90
larowlannitpick/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
Comment #91
smustgrave CreditAttribution: smustgrave at Mobomo commentedCan work on a 9.5 patch once this lands.
Comment #92
DeepaliJ CreditAttribution: DeepaliJ at Salsa Digital commentedApplied 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:
Comment #93
sonam.chaturvedi CreditAttribution: sonam.chaturvedi at QED42 for Drupal India Association commentedVerified 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.
Comment #96
smustgrave CreditAttribution: smustgrave at Mobomo commentedClosed #3103506: Layout Builder Add Block filter doesn't update after changing a matching string to a shorter matching string as a duplicate and moved over credit.
Comment #97
dmytro-aragornHere is a re-roll for the 9.5.3 version
Comment #98
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #99
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #100
Vigneshn CreditAttribution: Vigneshn as a volunteer commentedComment #101
Vigneshn CreditAttribution: Vigneshn as a volunteer commentedComment #102
smustgrave CreditAttribution: smustgrave at Mobomo commented@Vigneshn thank you for the interest but you hid the working patch for 10.1
Also expected you test a patch before uploading.
Comment #103
larowlanextreme nit pick ™️ - greater than 80 chars here
other than that I think this is ready
Comment #104
smustgrave CreditAttribution: smustgrave at Mobomo commentedaddressed 103
Comment #105
larowlanAdded screenshots to issue summary
Comment #108
lauriiiCommitted 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.
Comment #110
Rishabh Vishwakarma CreditAttribution: Rishabh Vishwakarma at OpenSense Labs for DrupalFit commentedAdding patch for 9.5.x as requested in #108
Comment #111
smustgrave CreditAttribution: smustgrave at Mobomo commented#110 failed the build.
Comment #112
Rishabh Vishwakarma CreditAttribution: Rishabh Vishwakarma at OpenSense Labs for DrupalFit commentedSolving CCF from#110
Comment #113
Rishabh Vishwakarma CreditAttribution: Rishabh Vishwakarma at OpenSense Labs for DrupalFit commentedSolving CCF from#112
Comment #114
R.shaikh CreditAttribution: R.shaikh commentedComment #115
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedRe-rolled patch for 9.5.x, attached interdiff for same.
Comment #116
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #118
lauriiiCommitted 06eac4f and pushed to 9.5.x. Thanks!