Charcoal-SE / userscripts

Collection of userscripts that are used by/are useful to Charcoal.
https://charcoal-se.org/scripts
Apache License 2.0
28 stars 27 forks source link

Updating MS Dark Theme #206

Open CoconutMacaroon opened 2 years ago

CoconutMacaroon commented 2 years ago

The MS Dark Theme userscript appears to be be unmaintained. When I tried it, a decent number of MS UI elements were still light-themed.

I'd like to start maintaining it and updating it to be more comprehensive of the MS UI

@ArtOfCode- seems to be the maintainer, but that script doesn't appear to have been updated for several years (at least here). If it is being maintained elsewhere and I've missed it - let me know. I don't want to take it from you if you're still maintaining it

If nobody objects, I'll likely work on it soon and probably send a PR

Thoughts?

ArtOfCode- commented 2 years ago

No objection here.

CoconutMacaroon commented 2 years ago

TODOs for me

makyen commented 2 years ago

@CoconutMacaroon Please don't turn the "MS Dark Theme" userscript into a Swiss Army knife of changes which you'd like to see. Please keep it focused on doing just creation of a dark theme.

You're welcome to create an additional userscript which performs other changes. It's reasonable for us to host such a userscript in this repository, if you desire.

However, given that we have complete control of the content on MS, I suggest trying to make such changes either as integrated into MS or in a way that allows them to be integrated into MS in the future. That does, however, require trying to reconcile the multiple points of view as to why things are done the way which they are. Sometimes there are reasons. Sometimes there are diverging opinions. Sometimes the only reason is because that was the way it happened to be implemented. Reconciling such things may require creating options which user could set, or taking a more complete look at the design.

An example is your desire to move the search and flagging links back into the topbar. I agree that they should be there. However, when moving them, we should take into account the reason they were moved out in the first place. Those links were moved out because one developer found the topbar to be poorly formatted at the viewport width which they were typically using. What should really happen is that a more detailed look should be taken at the topbar to make it more fully responsive, such that it looks and functions well at all viewport widths.

CoconutMacaroon commented 2 years ago

@makyen Yes, I fully agree. Unfortunately, I'm not familiar with how MS was written and so making changes to MS itself isn't something I can easily do. Regarding me moving those two buttons, yes, I believe I already have a TODO in there to remove it from this script and make it into a separate userscript. If I didn't add the todo, yes, I do intend to separate that out into a separate script for the reasons you mentioned

makyen commented 2 years ago

BTW: Please don't use something like Less in a userscript. That introduces a dependency into every single page for something that is merely a static transformation that can, and should, happen only once.

If you feel a need to use Less/SASS/SCSS, then a better solution is to keep a separate file which is translated and then included in the main userscript file as text. I strongly recommend against having it as a separate file which generates a .css file which you then include into the page as a dependency. That can and does break from time to time. It also makes development of the userscript substantially less convenient, because you have to actively manage the hosting of the resulting file and make sure that you're loading the appropriate version (release/production vs. development). We have had userscripts which have done that in the past (FIRE) and one which still does (SDS), but have/are actively moved/moving away from doing so due to the increased aggravation in development and the intermittent failures when the chosen hosting isn't available.

Also, please try to avoid using !important in CSS, unless really necessary. In a userscript, that's usually because something in the page uses it. Using !important so prolifically appears to be something which was part of how the original script built its CSS, so I understand that doing so is likely a holdover from that. It's also possible that every use is actually necessary, due to the CSS which is in the page (I haven't checked). I'm writing this paragraph merely because I happened to see !important used in multiple places in your prototype version and its use is often a red flag.

makyen commented 2 years ago

I also note that on review pages you're removing the space between the "False Positive" and "Skip" buttons when the "NAA" button isn't present. That space exists so that the position of the "Skip" button on the page doesn't move between reviewing questions and answers. As I recall, that button not moving between questions and answers was an explicitly requested feature.

CoconutMacaroon commented 2 years ago

That all seems reasonable, I've switched to plain CSS in this commit and I've reverted my toolbar change. I may put it in a separate script later, but for now I've just removed it. I've also put the gap back in the Review page.

Regarding !important, yes, I've used it quite a bit. I think I can fully not need it, but at the cost of having to have much more specific selectors. I'm not against doing that, per se, but it would take more time and add more complexity to the styles, but it would mean I wouldn't need !important. I can do that if you'd like, so let me know which you'd prefer.