search

calzoneman / sync

Node.JS Server and JavaScript/HTML Client for synchronizing online media
Other
1.46k stars 235 forks source link

Replace userlist visibility check logic #859

Closed Xaekai closed 4 years ago

Xaekai commented 4 years ago

JQuery queries using getComputedStyle, which makes it impossible to change userlist behavior using CSS. This replaces the check with a direct style="" value check so the JS does not trip up if any CSS customizations to the list visibility were made.

Example: if #userlist is set to display: block !important in the custom CSS, the toggle will break and won't toggle between none and block. Implementing custom CSS leveraging this state change thus is not possible. -- Algoinde

Algoinde commented 4 years ago

I pushed one more thing to my branch, which is a bit on tangent with CSS customization fixes, also a one-line fix. It adds a logged-in class on body so the descendants can be styled according to the login state.

calzoneman commented 4 years ago

@Algoinde can you explain what you plan to use .logged-in for? I'd like to avoid adding to the implicit contract of things customizations can depend on being there because it generates annoyance when I refactor things in such a way that it breaks those later.

Algoinde commented 4 years ago

@calzoneman yeah, sure. I have a statusbar right in the header using position: absolute and I wanted to control how wide it stretches to the right depending on the login fields being there or not. See custom css for https://cytu.be/r/Kaguya for the example.

I don't think the .logged-in class would cause refactor issues, it's a fairly basic thing that can be preserved across.

calzoneman commented 4 years ago

I don't think the .logged-in class would cause refactor issues, it's a fairly basic thing that can be preserved across.

The hidden assumption is that the channel page template will always know whether the user is logged in or not. The channel page template does not rely on this directly at all (only through the nav mixin).

Currently it is the case that logging in always sends the user to another page and redirects back, but one could imagine rewriting it to update the page inline, for example. Now the actual elements on the page don't match the CSS class anymore.

I don't want to sign up for the responsibility to make sure that this obscure use case continues to work; I'd advise you instead write your channel style in such a way that you don't depend on the user's login state to do CSS positioning.

Algoinde commented 4 years ago

I see. I still think that if a dynamic page update is done, the class can easily be added in the post-login handler, but this is your call. I will have to do JS for this, then, or relocate the login altogether. The elements are in different contexts and I have no way to link their state to each other.

What about the userlist?

calzoneman commented 4 years ago

the class can easily be added in the post-login handler, but this is your call.

It's more of a maintenance issue. (1) 3 months from now, I'll probably forget that this use case even exists, and if I were to make such a change it would break your CSS which is annoying. (2) Assuming I did remember, it's another piece of functionality to maintain, granted it's a small one by itself, but if I agreed to maintain behavior for little bits of channel CSS/JS that everyone that wanted it, it adds up.

Arguably I should have defined more clear boundaries around what is/is not supported, but anyways in the current situation we are in, my general stance is that I only maintain the functionality and markup that is used by core features of the site, and anything that is only used by channel customizations is left as an exercise to the reader.

What about the userlist?

The display visibility referenced in this PR sounds more like a bug fix to me. I'm ok with merging that part.

Algoinde commented 4 years ago

Aight, I see your point, no problem. Do I need to do anything with my pull request so only the first commit is in the pull request?

calzoneman commented 4 years ago

I think if you revert your latest commit and push it to your branch that could work.

Xaekai commented 4 years ago

I am not averse to embedding referable state statuses in the body element, but it would most certainly need to be a first-class citizen mechanism based on a MutationObserver, and the state would be via data-attributes, and is a project well outside the scope of a bugfix PR. But even beginning to think about how state is supposed to be accessible begs the question of ARIA compliance, which is enough to make anyone run and hide.