search

toolbox-team / reddit-moderator-toolbox

Moderator toolbox for reddit extension development.
https://www.reddit.com/r/toolbox
Apache License 2.0
114 stars 37 forks source link

Decouple Toolbox dark theme styles from RES nightmode #486

Open eritbh opened 3 years ago

eritbh commented 3 years ago

Asked for here and at least one other time that I can't find a link for right now.

Toolbox should provide its own toggle for light/dark theme, rather than relying only on RES's .res-nightmode class for it. We can probably still have an option to base toolbox's nightmode on RES's nightmode, but since RES doesn't apply night mode class/styles on new Reddit and there's no good way to get the new Reddit theme from jsAPI, it probably makes the most sense to just add a manual toggle in general settings.

We can probably just replace-all .res-nightmode to .tb-darkmode in our existing CSS, then add an option that will always add that class to the page, and another option to add that class to the page whenever RES's class is already there.

This will have an impact on subs that use custom styles for Toolbox elements, but if we default the RES sync option to enabled, then there won't be any immediately breaking changes.

creesch commented 3 years ago

Having a explicit toggle makes sense to me. However I'd like to approach it a bit differently in regards to this bit you proposed.

We can probably just replace-all .res-nightmode to .tb-darkmode in our existing CSS, then add an option that will always add that class to the page, and another option to add that class to the page whenever RES's class is already there.

I rather keep .res-nightmode alongside a new class we create for a few reasons:

eritbh commented 3 years ago

people currently having RES nightmode enabled with the expectation that it will also turn toolbox dark will suddenly find that this is no longer working

This is why I proposed we add a second option that's enabled by default that would actively look for the RES nightmode class and add our own dark theme class as well. That way, current nightmode users won't have their workflow broken.

The goal of having our own class would just be to avoid having a bunch of duplicated CSS selectors like .res-nightmode .stuff, .toolbox-darkmode .stuff.

RES is much quicker to load and apply the class (due to not having to do a variety of checks) meaning that there never is a bright "flash"

Since our CSS only impacts elements we add to the page ourselves, this doesn't really matter - as long as we apply our own class before we start adding toolbox UI to the page, we won't introduce a flash. RES always loading faster than Toolbox actually makes things easier for us; it means we don't have to delay our init to check if RES nightmode is enabled.

creesch commented 3 years ago

Since our CSS only impacts elements we add to the page ourselves, this doesn't really matter - as long as we apply our own class before we start adding toolbox UI to the page, we won't introduce a flash. RES always loading faster than Toolbox actually makes things easier for us; it means we don't have to delay our init to check if RES nightmode is enabled.

I just don't see why we would build and rely on logic here when it can be achieved with no active logic where the the only downside is a bunch of double selectors that's all. Other than that I don't care too much how it is done to be frank.

eritbh commented 3 years ago

I'm not particularly attached to the active logic solution either, wouldn't mind implementing it either way. The main benefit would probably just be for people using sub CSS to style Toolbox, since it makes the selectors simpler for them. Doing it with active logic would also mean that you could have RES nightmode enabled without toolbox being in dark mode if you wanted - though that might not be super useful, and might actually be undesirable from a support perspective. (Of course, if that isn't something we want to be possible, using active logic means we have a lot more flexibility in how we implement that sort of thing.)