Drop active class from listItems

[clarkwinkelmann]

This is something I've noticed while reviewing flarum/framework#2255 but isn't essential to change during the rewrite itself.

I notice we have some hacky code regarding isActive in LinkButton. The whole reason that code exists seems to be so listItems can access the active state from one level higher.

This is undesirable in our effort to keep the data flowing down components and I have not found any concrete reason why we need the active state to be accessible one level higher than the button.

The code I suggest removing is here https://github.com/flarum/core/blob/47f3ee0ce2de145de8aec4eb8043d62d4027f000/js/src/common/helpers/listItems.js#L34

This will in turn allow us to get rid of the horrible initAttrs and isActive method in https://github.com/flarum/core/blob/a298457fb72c738af0c366bec3e4342189c69075/js/src/common/components/LinkButton.js#L19 and instead just compute active in the view.

This will require a bit of additional code to add the active class to the button itself, which is currently not the case. We do have an active="true" HTML attribute applied to the button but I don't think that's actually used and might be a side effect of having it as prop in the first place. I've seen an extra line of code was added just to preserve that behavior but I don't think it's actually needed https://github.com/flarum/core/blob/a298457fb72c738af0c366bec3e4342189c69075/js/src/common/components/LinkButton.js#L26

The other changes required will be in the CSS. It seems right now all the selector are in the format li.active > a, so those could basically be replaced with a.active with no side effects. I've identified the following references:

https://github.com/flarum/core/blob/47f3ee0ce2de145de8aec4eb8043d62d4027f000/less/admin/AdminNav.less#L44-L58

https://github.com/flarum/core/blob/47f3ee0ce2de145de8aec4eb8043d62d4027f000/less/common/sideNav.less#L36-L40

https://github.com/flarum/core/blob/47f3ee0ce2de145de8aec4eb8043d62d4027f000/less/common/Dropdown.less#L69-L73

https://github.com/flarum/core/blob/47f3ee0ce2de145de8aec4eb8043d62d4027f000/less/common/Dropdown.less#L204-L211

I think some of this CSS will actually be completely removable since Button.active already has a style that's barely used (if at all) at the moment

[askvortsov1]

I completely agree with this: I'm not sure what the benefit of placing active in the <li> as opposed to the element itself is...

[clarkwinkelmann]

I think one possible reason is that libraries like Bootstrap used to place classes like that on <li>s instead of the link in the li in contexts like dropdowns. I can't find an example for active but here's an example for disabled https://getbootstrap.com/docs/3.4/components/#dropdowns-disabled

So having access to the class at that level could be useful to use some CSS frameworks, but I think that practice is being phased out.

I'm particularly against styling link containers based on attributes like those because it leads to those annoying designs where for example you hover a list item and it renders like a link, but the link is actually just a portion of it and the animation doesn't reflect the true link hitbox (I don't think Flarum actually has any instance of this though). Placing all classes on the link itself forces theme designers to style the actual element that can be interacted with.

[dsevillamartin]

I agree - this behavior isn't desirable and some trickery was needed to get it to work with mithril 2.

[franzliedke]

I'm not sure what the benefit of placing active in the <li> as opposed to the element itself is...

It does feel logical to me to mark list items as active (for native <select>s, this would be the selected elementI).

But interestingly, for the semantic equivalent aria-current, placing it on the links seems recommended - or at least demonstrated that way in this example.

Definitely makes sense to improve the data flow here. :+1:

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[clarkwinkelmann]

Still relevant

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!