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

Add exactActiveClass to A component #245

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

emdede
Copy link

@emdede emdede commented Feb 22, 2023

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 and exactActive. But you cannot independently provide classes for each of the three since you have to choose between one of the two states active and exactActive by using the end 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 the end property.

Considerations

Imo, treating active and exactActive as two distinct states is the wanted behavior because of a possible class pollution.

Simple example:

<A href="/main/nested" activeClass="text-white" exactActiveClass="text-red" />

With inclusive matching, this will generate an a element with class text-white text-red. It pollutes the space and also leads to class conflicts. A solution to make sure the exactActive classes are applied would be for the user to make them important 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 the exactActiveClass 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 be text-white

<A href="/home" exactActiveClass>Home</A> enables strict matching => class will either be active or exactActive

<A href="/home" exactActiveClass="text-white">Home</A> enables strict matching => class will either be active or text-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 and active 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.

@emdede
Copy link
Author

emdede commented Feb 22, 2023

I realized that with my initial implementation, even though end was marked deprecated it didn't actually account for the new matching paradigm, so I removed it altogether for now.

src/components.tsx Outdated Show resolved Hide resolved
@bikeshedder
Copy link

How about deprecating the end property and adding both an exactActiveClass and startActiveClass instead?

That way it would not be a breaking change, but future versions of the library could remove the end property which is superseeded by the exactActive and startActive memos. active could just stay as is and be a combination of exactActive | startActive.

Alternative names for startActive could be startsWithActive, prefixActive or parentActive.

@emdede
Copy link
Author

emdede commented Feb 23, 2023

@bikeshedder
I think it would be the most explicit way and I initially wanted to go that direction. In the end I didn't go with it because giving it a proper name that's convenient to use but conveys exactly what it is poses a challenge in and of itself :D. But for the behavior that I implemented now it's fine to keep activeClass because fine-grained activity is opt-in. By default, links match loosely and strictly at the same time. Ultimately, though, I would agree that exactMatch and partialMatch should be treated as two separate things.

@bikeshedder
Copy link

I like the names exactMatch and partialMatch. </bikeshedding>

@emdede
Copy link
Author

emdede commented Apr 11, 2023

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)

@ryansolid
Copy link
Member

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 end property is less than ideal. The whole handling of what is active in the Anchor has led to all this permutation for what should be a simple link. So trying to think a bit wider outside of the box.

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.

3 participants