-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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
Change "whitelist" to "allowlist" #31066
Conversation
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.
Please don't merge I want to have a closer look.
From a quick look, the change needs to be listed in the migration page in detail. I will check later the branch. |
ah, was thinking about migration guide, but then thought that it's not really a user/developer facing object (unless somebody was hooking directly into the function for some reason). |
Yeah, it's a breaking change so it needs to be documented. :) |
Pending the Migration addition, otherwise the changes look good. |
added migration guide entry ... too detailed or ok? @XhmikosR |
I'd like @Johann-S opinion, not sure if all of the listed methods are public. We should only list public stuff. AFAICT only |
explicitly mention the change for tooltips and popovers
Co-authored-by: Mark Otto <[email protected]>
Co-authored-by: Mark Otto <[email protected]>
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.
Thanks for this!
@Johann-S thoughts on the migration guide question? does it look good or does it need to be expanded? |
LGTM on the migration guide, since it's a public option it's important to be added in our migration guide |
Co-authored-by: XhmikosR <[email protected]> Co-authored-by: Mark Otto <[email protected]>
Note that stylelint removed |
Closes #31064
note that this leaves intact the use of
blacklist
for stylelint