-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(overlay): error when trying to add/remove empty string class #14919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
67729cc
to
5f6d112
Compare
src/cdk/overlay/overlay-ref.ts
Outdated
@@ -471,7 +471,10 @@ export class OverlayRef implements PortalOutlet, OverlayReference { | |||
|
|||
coerceArray(cssClasses).forEach(cssClass => { | |||
// We can't do a spread here, because IE doesn't support setting multiple classes. | |||
isAdd ? classList.add(cssClass) : classList.remove(cssClass); | |||
// Also trying to add an empty string to a DOMTokenList will throw. | |||
if (cssClass !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be if (cssClass)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's super edge-case-ey, but it would technically exclude a class called false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that's true; the string 'false'
is truthy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean somebody passing in false
as a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually expect doing nothing in that case over adding a 'false'
CSS class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
Fixes an error being thrown if an empty string is passed as one of the values to `addPanelClass` or `removePanelClass`.
5f6d112
to
176c4cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes an error being thrown if an empty string is passed as one of the values to `addPanelClass` or `removePanelClass`. (cherry picked from commit 5509c23)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes an error being thrown if an empty string is passed as one of the values to
addPanelClass
orremovePanelClass
.