When Olivero's desktop (wide) search form is open, pressing the Escape key should close it.

CommentFileSizeAuthor
#29 3209125-olivero-close-search-escape-29.patch1.14 KBaaron.ferris
#23 3209125-23.patch1.15 KBpaulocs
#21 3209125-olivero-close-search-escape-20.patch1.08 KBaaron.ferris
#20 3209125-after_patch.mp4394.66 KBabhijith s
#17 3209125-17.patch1.09 KBtushar_sachdeva
#17 interdiff_13-17.txt1.95 KBtushar_sachdeva
#16 screen-capture.mp41.84 MBsakthivel m
#11 interdiff_8-11.txt1.54 KBtushar_sachdeva
#11 3209125-11.patch1.26 KBtushar_sachdeva
#8 3209125-8.patch1.45 KBkapilv
#7 interdiff-3-6.txt1.38 KBkishor_kolekar
#7 search-esckey-js-3209125-6.patch1.45 KBkishor_kolekar
#3 Screen Recording 2021-04-19 at 12.00.38 PM.mov1.99 MBtushar_sachdeva
#3 search-esckey-js-3209125-3.patch725 bytestushar_sachdeva

Comments

mherchel created an issue. See original summary.

tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: Unassigned » tushar_sachdeva
tushar_sachdeva’s picture

tushar_sachdeva commented
StatusFileSize
new search-esckey-js-3209125-3.patch725 bytes
new Screen Recording 2021-04-19 at 12.00.38 PM.mov1.99 MB

Quick and easy patch attached. For ref, video is attached when a KeyboardEvent fire, you can test which keycode is pressed as for ESC key it's 27.

tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: tushar_sachdeva » Unassigned
tushar_sachdeva’s picture

tushar_sachdeva commented
Status: Active » Needs review
kishor_kolekar’s picture

kishor_kolekar commented
Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

kishor_kolekar commented
StatusFileSize
new search-esckey-js-3209125-6.patch1.45 KB
new interdiff-3-6.txt1.38 KB

please review

kapilv’s picture

kapilv commented
StatusFileSize
new 3209125-8.patch1.45 KB
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

Tests are failing, but here's a quick review of #7

  1. +++ b/core/themes/olivero/js/search.es6.js
@@ -2,6 +2,14 @@
+    console.log(event.which);

    Errant console. 

  2. +++ b/core/themes/olivero/js/search.es6.js
@@ -2,6 +2,14 @@
+    if (event.keyCode == 27) {

    Instead of e.keycode, use e.key === 'Escape' || e.key === 'Esc' 

  3. +++ b/core/themes/olivero/js/search.es6.js
@@ -2,6 +2,14 @@
+      searchWideWrapper.classList.remove('is-active');

    Instead of manually changing attributes, use the toggleSearchVisibility function.

Be sure to run yarn prettier after your edits to make sure prettier doesn't fail.

tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: kishor_kolekar » tushar_sachdeva
tushar_sachdeva’s picture

tushar_sachdeva commented
StatusFileSize
new 3209125-11.patch1.26 KB
new interdiff_8-11.txt1.54 KB

The new patch with interdiff is attached, with the changes mentioned above in #9. Please review and verify it.

tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: tushar_sachdeva » Unassigned
tushar_sachdeva’s picture

tushar_sachdeva commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

Getting an error within CI 

  7:7  error  'toggleSearchVisibility' was used before it was defined  no-use-before-define
tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: Unassigned » tushar_sachdeva
sakthivel m’s picture

sakthivel m commented
Status: Needs work » Needs review
StatusFileSize
new screen-capture.mp41.84 MB

Hi @mherchel

when i pressed ESC key, the search form popup closed. it's working fine in without patch, Attached video here.

Please correct me if i missed anything here

tushar_sachdeva’s picture

tushar_sachdeva commented
StatusFileSize
new interdiff_13-17.txt1.95 KB
new 3209125-17.patch1.09 KB

Hope this will work fine , please review .

aaron.ferris’s picture

aaron.ferris commented

Patch from #17 applies cleanly for me and does what id expect it to do.

Would we want to keep the es6 file consistent in the usage of arrow functions?

IE:

  document.addEventListener('keydown', (e) => {
    if (e.key === 'Escape' || e.key === 'Esc') {
      toggleSearchVisibility(false);
    }
  });
tushar_sachdeva’s picture

tushar_sachdeva commented
Assigned: tushar_sachdeva » Unassigned
abhijith s’s picture

abhijith s commented
StatusFileSize
new 3209125-after_patch.mp4394.66 KB

Applied patch #17 and it works.The search form is closing when pressing the ESC key.Screen recording added.

RTBC +1

aaron.ferris’s picture

aaron.ferris commented
StatusFileSize
new 3209125-olivero-close-search-escape-20.patch1.08 KB

Attaching a patch based off @tushar_sachdeva's patch in #17 but with an arrow function in the es6 file for consistency.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

This is looking really close. Thanks everyone for working on it!

+++ b/core/themes/olivero/js/search.es6.js
@@ -29,6 +29,12 @@
+  document.addEventListener('keydown', (e) => {

Lets use the keyup event. The only reason being consistency (we're using keyup in navigagtion.es6.js and second-level-navigation.es6.js)

+++ b/core/themes/olivero/js/search.es6.js
@@ -29,6 +29,12 @@
   Drupal.olivero.toggleSearchVisibility = toggleSearchVisibility;

Move the new code below this line. We want the reference to toggleSearchVisibility to appear directly below where it was originally defined.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
StatusFileSize
new 3209125-23.patch1.15 KB

Here is the patch addressing comment #22.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

I really don't know what triggered the error.

tushar_sachdeva’s picture

tushar_sachdeva commented

@paulocs make sure that you have run yarn prettier after your edits to make sure prettier doesn't fail.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

I ran it. I ran yarn prettier but found no problem in the lines that I added in the patch.
Maybe I'm doing it wrong because its my first time using it.
When I run it, it fixes other lines in the file but there are no change in the lines that I added.

aaron.ferris’s picture

aaron.ferris commented

I can’t see anything obvious wrong with the patch, the output of the failure suggests the file isn’t supported.

aaron.ferris’s picture

aaron.ferris commented
StatusFileSize
new 3209125-olivero-close-search-escape-29.patch1.14 KB

Weird, but I think its a problem with the files not matching. Even with the es6 file at default the test fails locally. I've started from scratch, yarn run build:js and attaching a patch that should pass

Local:
Checking core/themes/olivero/js/search.es6.js
core/themes/olivero/js/search.es6.js passed

Checking core/themes/olivero/js/search.js
core/themes/olivero/js/search.js passed

Did you run yarn run build:js locally? That's the only thing I can think of.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

#29 looks good to me. Thanks again for working on this.

  • lauriii committed 1af77ea on 9.2.x 
    Issue #3209125 by tushar_sachdeva, aaron.ferris, kishor_kolekar, paulocs...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Priority: Minor » Normal
Status: Reviewed & tested by the community » Fixed

Committed 1af77ea and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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