Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating MS Dark Theme #206

Open
CoconutMacaroon opened this issue Oct 5, 2022 · 7 comments
Open

Updating MS Dark Theme #206

CoconutMacaroon opened this issue Oct 5, 2022 · 7 comments

Comments

@CoconutMacaroon
Copy link

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-
Copy link
Member

No objection here.

@CoconutMacaroon
Copy link
Author

CoconutMacaroon commented Oct 6, 2022

TODOs for me

  • Fix font colors on charts, remove "glow" effect on some charts
  • Ensure suitable contrast ratios on text
    S
  • Separate logic for moving search/flagging into separate userscript - removed for now, I may create a separate script for it later
  • Decide on Less vs. plain CSS - per makyen's request, I've opted for plain CSS as so far everything works in plain CSS

@makyen
Copy link
Contributor

makyen commented Oct 6, 2022

@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
Copy link
Author

CoconutMacaroon commented Oct 6, 2022

@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
Copy link
Contributor

makyen commented Oct 6, 2022

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
Copy link
Contributor

makyen commented Oct 6, 2022

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants