From code review #73 at #3111409: Add new Olivero frontend theme to Drupal 9.1 core as beta.

There is a props.body.classList.remove('js-overlay-active', 'js-fixed') that should be 2 lines for IE11 compat. (only one argument supported for the remove function).

CommentFileSizeAuthor
#8 3173905-8.patch1.47 KBkostyashupenko
#6 interdiff_3_5.txt1.41 KBkapilv
#5 3173905-5.patch916 bytesravi.shankar
#3 olivero-3173905-3.2.patch762 bytesnod_

Comments

mherchel created an issue. See original summary.

mherchel’s picture

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

Note I think that the node.classList.remove() 2nd param works properly in IE11. I think @nod_ is thinking of the node.classList.toggle() method.

Either way, we'll need to either fix it, or document that it's working properly in IE11.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: JavaScript: node.classList.remove() only supports one property » JavaScript: node.classList.remove() only supports one argument
Status: Active » Needs review
StatusFileSize
new olivero-3173905-3.2.patch762 bytes

Ok it was an extra line, same thing is done in the toggleNav function.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Title: JavaScript: node.classList.remove() only supports one argument » Olivero: node.classList.remove() only supports one argument
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
Status: Needs review » Needs work

The last patch removes the functionality to remove the overlay on resize. Not sure if that's the appropriate behavior.

ravi.shankar’s picture

ravi.shankar commented
StatusFileSize
new 3173905-5.patch916 bytes

Added reroll of patch #3.

kapilv’s picture

kapilv commented
Status: Needs work » Needs review
StatusFileSize
new interdiff_3_5.txt1.41 KB
mherchel’s picture

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

Need to address the feedback in #4

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
StatusFileSize
new 3173905-8.patch1.47 KB

It is always interesting to check if thing is buggy in IE11. Well, docs says IE doesn't support multiple arguments for add() or remove() https://developer.mozilla.org/en-US/docs/Web/API/Element/classList#Brows..., but for some reason the following code props.body.classList.remove('js-overlay-active', 'js-fixed'); works fine in my real IE11 and MS Edge. Then i've tried to assign/remove classes from IE 11 console and it was fully unsuccessful -> means, doesn't work with multiple arguments. So to me it looks like it works only for some mysterious reason, sometimes.

Better separate this code on two lines and keep everything in the "core" style.

Also fixed some little error during yarn lint:core-js

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Better to be safe than sorry where IE11 is concerned, #8 looks good to me.

  • lauriii committed 63a3488 on 9.2.x 
    Issue #3173905 by nod_, ravi.shankar, kostyashupenko, mherchel: Olivero...

  • lauriii committed 2c007ca on 9.1.x 
    Issue #3173905 by nod_, ravi.shankar, kostyashupenko, mherchel: Olivero...
lauriii’s picture

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

Committed 63a3488 and pushed to 9.2.x and 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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