Olivero header-search-wide.pcss.css and header-search-narrow.pcss.css adjustments

Patch attached!

Rerolled the patch in #8 and fixed the custom commands failure errors, thanks!

Updated patch 11 to fix failed custom commands

HI all,

Rolling up patch 13 to fix failed custom commands Failed in patch #12, which i found after run post css command please review.

Thanks

Re-roll of #13 attached.

@mherchel, the Semicolon is missing for some CSS properties in patch #14.

-[dir="ltr"] .search-narrow__wrapper input[type="search"] {
+[dir="ltr"] .block-search-narrow input[type="search"] {
     padding-left: 1.125rem
 }
 
-[dir="rtl"] .search-narrow__wrapper input[type="search"] {
+[dir="rtl"] .block-search-narrow input[type="search"] {
     padding-right: 1.125rem
 }
 
-[dir="ltr"] .search-narrow__wrapper input[type="search"] {
+[dir="ltr"] .block-search-narrow input[type="search"] {
    padding-right: 1.125rem
 }
 
-[dir="rtl"] .search-narrow__wrapper input[type="search"] {
+[dir="rtl"] .block-search-narrow input[type="search"] {
     padding-left: 1.125rem
 }

@Gauravmahlawat that file is automatically generated from the front-end tooling. See this stackoverflow comment regarding semicolons in CSS blocks

Patch #14 applies cleanly and seems to have passed all the tests, however I wanted to point out that the issue description recommended implementing .block-search--wide, I assume to use BEM standards, but the patch currently implements .block-search-wide essentially creating a new BEM block.

If that's not an issue, then +1 for RTBC

Thanks for the review!

I wanted to point out that the issue description recommended implementing .block-search--wide, I assume to use BEM standards, but the patch currently implements .block-search-wide essentially creating a new BEM block.

Yeah, thats intentional. To me the differences are enough that it warrants a new block. We also have the code split out into multiple CSS files.

Review and requested changes incoming

1. +++ b/core/themes/olivero/js/search.es6.js
   @@ -1,6 +1,8 @@
   +  const searchWideButton = document.querySelector('.block-search-wide__button');
   

   While we're refactoring, let's add data attributes onto all of the selectors that JavaScript is querying. This is per Drupal coding standards (see #3153234: [Meta] Olivero JavaScript should be selecting [data-drupal-selector] attributes where possible).

   Something like data-search-wide-button, etc

2. +++ b/core/themes/olivero/js/search.es6.js
   @@ -40,13 +42,15 @@
   +        '.block-search-wide__wrapper, .block-search-wide__wrapper *',
   

   This can also use the data attribute selectors

Thanks, @mherchel for the patch, and Thanks @andy-blum for the review.

Kindly review the updated patch based on #19.

Thanks!

There's a prettier error on the patch. Run yarn prettier to fix it.

Other than that, it looks great!

Re-rolled patch #20.
Attached interdiff for the same.

Please ignore a/core/misc/ajax.js | a/core/misc/jquery.cookie.shim.js in interdiff. these files are not included in patch.

The interdiff in #24 contains some irrelevant changes that are not in the actual patch. I'm attaching the correct interdiff between #20 and #24 here.

Passes tests, and looks perfect. RTBC!

I also adjusted the credits.

1. +++ b/core/themes/olivero/css/components/header-search-wide.pcss.css
   @@ -182,16 +182,16 @@
   +.block-search-wide__button {
   
   @@ -226,14 +226,12 @@
   +    & .block-search-wide__button-close {
   

   Does this have to be nested still?

2. +++ b/core/themes/olivero/templates/block/block--secondary-menu--plugin-id--search-form-block.html.twig
   @@ -35,14 +35,14 @@
   +    <div{{ content_attributes.addClass('block-search-wide__wrapper').setAttribute('data-search-wide-wrapper', '') }}>
   

   Is there a specific reason not to use data-drupal-selector which is something we have been trying to standardize on?

+++ b/core/themes/olivero/css/components/header-search-wide.pcss.css
@@ -182,16 +182,16 @@
+.block-search-wide__button {

@@ -226,14 +226,12 @@
+    & .block-search-wide__button-close {

Does this have to be nested still?

Yep. It's nested under .block-search-wide__button[aria-expanded="true"], which is necessary to turn the icon into an X

+++ b/core/themes/olivero/templates/block/block--secondary-menu--plugin-id--search-form-block.html.twig
@@ -35,14 +35,14 @@
+    <div{{ content_attributes.addClass('block-search-wide__wrapper').setAttribute('data-search-wide-wrapper', '') }}>

Is there a specific reason not to use data-drupal-selector which is something we have been trying to standardize on?

I didn't know about using data-drupal-selector until recently. We'll make the change here.

Updated patch and interdiff attached with changes to the JavaScript selector pattern.

@mherchel Thanks for the updated patch, it's applied successfully and working as expected.

Attaching just SS for reference instead of video.

Thanks!

Committed 25de809 and pushed to 9.3.x and 9.2.x. Thanks!