-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Check for match in stylesheet host and protocol #20673
Conversation
Size Change: +319 B (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
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.
Looks good. I think it might be nice to use the url package functions as they're unit tested. Having said that, i threw a few things at the regex in this PR and didn't spot any issues.
styleSheet.href.includes( window.location.hostname ) | ||
matcheableUrl && | ||
matcheableUrl.length > 2 && | ||
matcheableUrl[ 1 ] === window.location.protocol && |
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.
The url package has a getProtocol
function that might be useful as this could be simplified to getProtocol( styleSheet.href ) === window.location.protocol
.
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.
Oooh great tip, thanks! Updated.
matcheableUrl && | ||
matcheableUrl.length > 2 && | ||
matcheableUrl[ 1 ] === window.location.protocol && | ||
matcheableUrl[ 2 ] === window.location.host |
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.
The url package also has getAuthority
function, which seems like it returns the same url part as the window.location.host (subdomain.domain:port
).
Description
Addresses #20632.
How has this been tested?
Tested in several browsers (including IE) and ran automated tests.
Screenshots
Types of changes
Checklist: