-
Notifications
You must be signed in to change notification settings - Fork 341
Allow to opt-in to domains when excluded via glob #1970
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
base: master
Are you sure you want to change the base?
Conversation
API.styles.toggleOverride(menu.styleId, rule, false, input.checked); | ||
}; | ||
} | ||
for (const el of $$('[data-include]')) { |
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.
Great, but could you combine the two loops using $$('[data-exclude], [data-include]') ?
if ((ovr = style.inclusions) && ovr.some(urlMatchExclusion, query)) { | ||
return 'included'; | ||
} | ||
if ((ovr = style.exclusions) && ovr.some(urlMatchExclusion, query)) { |
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 think it's a good thing to drastically change the priority of matching. I guess it may be a user configurable option in the style exposed in style settings in the editor and in the popup's three-dot menu.
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.
yeah, I agree with you and that's what my comment about backwards compatibility was.
I can't really imagine a situation where an excluded glob is superceding an included line, because then surely your included line just isn't useful?
But yeah I'de agree that user configurable is a possibility
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.
To clarify, the problem is that now the conditions like style.enabled
are checked prior to inclusion/exclusion, which messes up matching.
|
||
export function urlMatchStyle(query, style) { | ||
let ovr; | ||
if ((ovr = style.exclusions) && ovr.some(urlMatchExclusion, query)) { |
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.
See my comment about priority.
This allows wildcarding excluded all domains for a stylesheet (like global dark mode enable), & then opting in per-domain when I need it.
Related: #1892
Basically just prioritize matching includes over excludes. I don't think it's backwards compatible.