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

Kirin dark mode #511

Closed
wants to merge 2 commits into from
Closed

Kirin dark mode #511

wants to merge 2 commits into from

Conversation

ben
Copy link
Owner

@ben ben commented Oct 13, 2022

@kirincorleone suggested a browser-plugin hack on Discord, this is an attempt to make it a theme in the box.

  • Copy-paste in some stuff
  • Make it better
  • Maybe reboot and finish what @rsek started with CSS vars
  • Update CHANGELOG.md

@ben
Copy link
Owner Author

ben commented Oct 13, 2022

Looks like the original relies on some styles from Darkreader. I wasn't able to find a CSS file I could copy-paste straight away, but maybe this is a good time to start thinking about reorganizing our styles into something that's easily overridable. We're halfway there with the CSS vars, but it'd be nice to do the whole thing.

@rsek
Copy link
Collaborator

rsek commented Oct 13, 2022

couple points:

  • there's built-in flag for dark-mode. unless having it as a brand-new class enables some necessary functionality, it's probably a good idea to use the built-in flag instead. then it'll actually obey people's OS settings rather than requiring recourse to browser extensions (but it's still a good idea to include an option to override it -- people might want to style their OS dark and their sheet light, or vice versa)
  • ideally, being sensitive to dark mode is a property of a theme's colour scheme, independent of a theme's layout; in other words, a variation of a single theme, rather than a discrete theme in and of itself. this is part of the point of doing a bunch of CSS vars, and trying to build in some separation of concerns w/r/t layout vs. colourways. that seperation of concerns probably won't land with my initial PR, tho
  • after the initial CSS refactor PR, this could be a 25-line style sheet that sets a bunch of vars rather than a 225-line style sheet that calls out a bunch of stuff individually
  • "reboot" implies the refactor has ceased :) it's more accurate to say i can only tolerate doing that job for so long before doing something else, because it's long, involved, touches a lot of files, and is generally kind of tedious

so, as far as implications on that other draft PR... TBH, i'd probably be gutting this like i am everything else. if this new theme were committed, the main work it would add on my end is scratch-writing a theme to replace it (so as not remove functionality).

@ben
Copy link
Owner Author

ben commented Oct 13, 2022

Yeah, this was meant to be a spike. I forgot about your draft PR (#490), we should rebase it and get it merged in, because there's a lot of good stuff in there.

And yeah, each theme should ideally provide a light and dark variant: Ironsworn would be black-text-on-white and light-gray-text-on-black, and Starforged would be black-text-on-teal and teal-text-on-black.

@ben ben closed this Nov 19, 2022
@ben ben deleted the kirin-dark-mode branch November 19, 2022 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants