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

Always use restrictTo #44

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Always use restrictTo #44

wants to merge 7 commits into from

Conversation

ai
Copy link
Collaborator

@ai ai commented Nov 28, 2023

I suggest to always have allow-list of states, because right now we prefix :where(), :not() and many other CSS functions like this.

Even if we blacklist when, new will be added in the future. It is more likely to add new non-interactive pseudo-state.

@giuseppeg
Copy link
Owner

giuseppeg commented Nov 30, 2023

Good idea! I think that going for an allow-list approach makes sense.

In this case though we shouldn't use restrictTo in my opinion because if a user wants to add a pseudo class to the default ones they will have to rewrite interactiveStates by hand.

Since we want to go for an opt-in (allowlist) approach instead opt-out (blacklist), we should remove the concept of blacklist and restrictTo altogether and perhaps expose a custom option that is a function that should return an array of supported pseudo-classes. This function will get the list of default.

let defaultClasses = [':hover', ':active', ':focus', ':visited', ':focus-visible', ':focus-within'];

if (typeof options.custom === "function") {
   defaultClasses = options.custom(defaultClasses);
   if (false === Array.isArray(defaultClasses)) {
       throw new Error("options.custom: Invalid return value. The function should return an array of strings");
   }
}

@giuseppeg
Copy link
Owner

if you want options.custom can be polymorphic and be either the function above or an Array that will override everything

@ai
Copy link
Collaborator Author

ai commented Nov 30, 2023

@giuseppeg I updated API to custom

Copy link
Owner

@giuseppeg giuseppeg left a comment

Choose a reason for hiding this comment

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

Awesome thank you! We'll release 0.5.0 once all these changes are merged

@ai
Copy link
Collaborator Author

ai commented Nov 30, 2023

I need to merge it to master to make another PR

@ai
Copy link
Collaborator Author

ai commented Nov 30, 2023

I will add changes to that PR to avoid merging

@ai
Copy link
Collaborator Author

ai commented Nov 30, 2023

I also moved to modern JS syntax, but I think we need to merge #46 to fix CI.

But these 2 PRs is all changes which I am suggesting. We can merge them and release.

@giuseppeg
Copy link
Owner

Merged the other, feel free to rebase.

@ai ai force-pushed the refactor/always-restrictTo branch from f5fd3bc to a14ed72 Compare November 30, 2023 13:59
@ai
Copy link
Collaborator Author

ai commented Nov 30, 2023

Rebased and fixed. Merge and release when you will be ready.

@giuseppeg
Copy link
Owner

Thanks! Will do it tomorrow morning because I am not home

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
};

const plugin = function (options = {}) {
options.preserveBeforeAfter = options.preserveBeforeAfter ?? true;
Copy link
Owner

Choose a reason for hiding this comment

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

similarly to pseudo classes we can add support for pseudo elements, defaulting to :before and :after only, we would need a matching custom option similar to the one for pseudo classes to customize these

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we have done it in separated PR? I am not sure why we need to process :after at all.

Comment on lines +14 to 18
const blacklist = {
':root': true,
':host': true,
':host-context': true
};
Copy link
Owner

Choose a reason for hiding this comment

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

since now we are using an allowlist pattern, this is not necessary right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure to be honest. Do you remember why you added it?

ai and others added 3 commits December 11, 2023 17:21
Co-authored-by: Giuseppe <giuseppeg@users.noreply.github.com>
Co-authored-by: Giuseppe <giuseppeg@users.noreply.github.com>
Co-authored-by: Giuseppe <giuseppeg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants