Skip to content
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

Use classList.toggle #23273

Merged
merged 6 commits into from
Sep 14, 2021
Merged

Use classList.toggle #23273

merged 6 commits into from
Sep 14, 2021

Conversation

jelbourn
Copy link
Member

With Angular dropping IE11 support in version 13, we can now use classList.toggle with the second force param. Related to #7374

@jelbourn jelbourn added the target: major This PR is targeted for the next major release label Jul 29, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 29, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tslint rule for this that we should get rid of.

@jelbourn jelbourn requested a review from a team as a code owner July 29, 2021 17:00
@jelbourn jelbourn force-pushed the no-ie11-classlist branch from da1e9fc to 49d5e6d Compare July 29, 2021 17:24
@jelbourn jelbourn added the merge: preserve commits When the PR is merged, a rebase and merge should be performed label Jul 29, 2021
@jelbourn
Copy link
Member Author

Updated to remove tslint check

@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 29, 2021
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jul 29, 2021
@zarend zarend removed the action: merge The PR is ready for merge by the caretaker label Jul 30, 2021
@zarend
Copy link
Contributor

zarend commented Jul 30, 2021

this has a merge conflict, please resolve the conflict so we can run presubmit

@jelbourn jelbourn force-pushed the no-ie11-classlist branch from 49d5e6d to f3622ed Compare August 2, 2021 16:36
@jelbourn
Copy link
Member Author

jelbourn commented Aug 2, 2021

@zarend rebased

@andrewseguin andrewseguin added the action: merge The PR is ready for merge by the caretaker label Aug 5, 2021
@andrewseguin
Copy link
Contributor

This failed some targets because either the list of classes to add/remove is an empty array or the array contains an empty string, which produces SyntaxError: Failed to execute 'add' on 'DOMTokenList': The token provided must not be empty.

You can test this locally in the devtools by trying to add an empty list/token.

To fix, you'll want to filter out empty strings and only apply add/remove if the array has length != 0

@andrewseguin
Copy link
Contributor

To make this pass internally, only overlay needs to be changed to something like:

  /** Toggles a single CSS class or an array of classes on an element. */
  private _toggleClasses(element: HTMLElement, cssClasses: string | string[], isAdd: boolean) {
    const classes = coerceArray(cssClasses || []).filter(c => !!c);

    if (classes.length) {
      isAdd ? element.classList.add(...classes) : element.classList.remove(...classes);
    }
  }

With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
With Angular dropping IE11 support in version 13, we can now use `classList.toggle` with the second `force` param. Related to angular#7374
@jelbourn
Copy link
Member Author

@andrewseguin updated PR with the fix for overlay, including additional checks in the unit tests for this case.

@andrewseguin andrewseguin merged commit f4a54d6 into angular:master Sep 14, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: preserve commits When the PR is merged, a rebase and merge should be performed target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants