-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add exactActiveClass
to A component
#245
base: main
Are you sure you want to change the base?
Conversation
I realized that with my initial implementation, even though |
How about deprecating the That way it would not be a breaking change, but future versions of the library could remove the Alternative names for |
@bikeshedder |
I like the names |
Is there any particular reason for holding back on this? Current iteration has the deficit of not being able to distinguish between all of the 3 possible match states. If there's some implementation detail that needs to be changed or any other concerns, just let me know. (have to resolve conflicts first anyway, but waiting for input) |
The reason this has not been merged is that it made me wonder if handling active class in the link makes sense at all anyway. I do agree though the |
The problem
With current design of the
A
component classes, there is one shortcoming:An
A
component can be in one of three states:inactive
,active
andexactActive
. But you cannot independently provide classes for each of the three since you have to choose between one of the two statesactive
andexactActive
by using theend
property.What this PR does is add the
exactActiveClass
prop which makes it so that you can provide classes for each of the three possible states. It also deprecates theend
property.Considerations
Imo, treating
active
andexactActive
as two distinct states is the wanted behavior because of a possible class pollution.Simple example:
With inclusive matching, this will generate an
a
element with classtext-white text-red
. It pollutes the space and also leads to class conflicts. A solution to make sure theexactActive
classes are applied would be for the user to make themimportant
but it's unintuitive and pollutes the class space even more.Having this in mind, I think going for a stricter mutually exclusive distinction is the best way to go.
It does lead to duplication ( e.g.
activeClass="text-white text-lg" exactActiveClass="text-white text-xl"
) but at least there is nothing unexpected going on in this scenario.I'm open to suggestions and looking forward to feedback. Have a nice day
Update
I've edited some of the original post to keep it a bit more concise. I went with a non-breaking implementation and one that is a bit more explicit than it was before. Mostly because of the added complexity of allowing
true
to just pass theexactActiveClass
attribute. "3 distinct States" is opt-in.It would now work like this (disregarding
inactive
class):<A href="/home" activeClass="text-white">Home</A>
will match both partially and exactly and class will betext-white
<A href="/home" exactActiveClass>Home</A>
enables strict matching => class will either beactive
orexactActive
<A href="/home" exactActiveClass="text-white">Home</A>
enables strict matching => class will either beactive
ortext-white
This makes me think, wouldn't it be nice to have this same opt-in behavior for the other classes as well? Currently, there are
inactive
andactive
classes whether I need/want them or not. Would give a bit more control to users to have to opt-in first.<A href="/home" inactiveClass activeClass>Home</A>
But I'll leave that for a different PR/time.