solid-router icon indicating copy to clipboard operation
solid-router copied to clipboard

Add `exactActiveClass` to A component

Open emdede opened this issue 2 years ago • 6 comments

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 avatar Feb 22 '23 03:02 emdede

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.

emdede avatar Feb 22 '23 04:02 emdede

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.

bikeshedder avatar Feb 22 '23 17:02 bikeshedder

@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.

emdede avatar Feb 23 '23 01:02 emdede

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

bikeshedder avatar Feb 23 '23 06:02 bikeshedder

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)

emdede avatar Apr 11 '23 09:04 emdede

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.

ryansolid avatar Jan 30 '24 21:01 ryansolid