Spawned from #3253156: [meta] Remove IE11 Support from Olivero.

Internet Explorer does not support the second argument within Element.classList.toggle(). See https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle

Now that Drupal 10 does not support IE, we can slightly refactor the JS to make use of this second "force" parameter.

CommentFileSizeAuthor
#10 3255131-10.patch10.38 KBandy-blum
#7 3255131-2-7.txt3.06 KBandy-blum
#7 3255131-7.patch14.23 KBandy-blum
#2 3255131.patch7.31 KBmherchel

Issue fork drupal-3255131

Command icon 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:

About issue forks

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Active » Needs review
StatusFileSize
new 3255131.patch7.31 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented

I bet core needs separate issue to replace the usage as well, ++

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented

Logic appears to be maintained in the files that were edited, and I confirmed the classes & attributes toggle correctly in the browser. Did we want to include the second-level-navigation in this refactor as well? If not, good to move to RTBC.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

yeah that's a good call. We could include that here.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs review » Needs work
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3255131-7.patch14.23 KB
new 3255131-2-7.txt3.06 KB

New patch with second-level-nav logic updated.

mherchel’s picture

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

Thanks for working on this! Reviewed and manually tested the changes in #7.

mherchel’s picture

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

New patch has some unrelated CSS changes.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3255131-10.patch10.38 KB

Removed unrelated CSS changes

mherchel’s picture

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

Manually tested and looked through patch in #10. Looks perfect. Thank you!

droplet’s picture

droplet commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/olivero/js/search.es6.js
@@ -42,15 +42,13 @@
     searchWideButton.setAttribute('aria-expanded', visibility === true);
+    searchWideWrapper.classList.toggle('is-active', visibility);

after patch, these 2 lines are not using equal conditions.

xjm’s picture

xjm
she/her
Primary language English
commented
Priority: Normal » Major
Issue tags: +Drupal 10
xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Drupal 10 beta should-have

finnsky made their first commit to this issue’s fork.

finnsky opened merge request !2468

finnsky’s picture

finnsky commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

I think there is a type error in one line in the refactoring, otherwise this looks correct to me.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +js-novice
nod_’s picture

nod_
Primary language French
Location Lille
commented

Couple of comments to address in the MR

nod_’s picture

nod_
Primary language French
Location Lille
commented

Updated the MR so that it's using the correct js files following the mass-rename.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Works for me, thanks :)

  • lauriii committed a474384 on 10.1.x 
    Issue #3255131 by andy-blum, finnsky, mherchel, nod_: Olivero: Refactor...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed a474384 and pushed to 10.1.x. Thanks!

lauriii closed merge request !2468

Status: Fixed » Closed (fixed)

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