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

Add Dark & Auto color schemes #514

Merged
merged 36 commits into from
Sep 29, 2023
Merged

Conversation

JHWelch
Copy link
Contributor

@JHWelch JHWelch commented Sep 20, 2023

CleanShot 2023-09-26 at 17 56 40@2x

Summary

Fixes #403
Adds support for a colorScheme config value. Possible values are light, dark, and auto.

I left the default value as light trying to reduce the impact on existing users.

I'm not even a little attached to the colors I used, so please recommend new ones/update them. They are all available as CSS variables.
Colors updated to match GitHub

Changes to Consider

  • I have the "light" color scheme not apply a class to reduce the impact on existing users. I could change to add a class for consistency.
  • I ended up using variables for the dark mode colors because it made things much easier, but during that I converted the light mode colors to variables as well. I could revert that change 93103e2. However, I think it's a generally useful change because it makes it easier to change the colors in the future, and for users to override the colors in their own CSS.
  • Because of the nature of the demo page displaying light, dark & auto, I had trouble figuring out how to build a highlight.js theme that worked. I manually created a css file from the light & dark themes and then adjusted classes as appropriate 0de1e04. I don't think this is an ideal solution, but I couldn't figure out a better way to do it. I'm open to suggestions. Not super familiar with WebPack, but assume we could just build that file dynamically.

General Notes

  • Updated README for config & described process for linking two highlight.js themes for dark/light.
  • Refactored FileListRender into a class like the other renderers to be able to pass in the config. 4e6bb49

@JHWelch JHWelch marked this pull request as ready for review September 20, 2023 02:16
Copy link
Owner

@rtfpessoa rtfpessoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor comments.
Overall looks ok.
Can you also grab the colors from GitHub I think they have a pretty nice color scheme and it would look better.

src/templates/file-summary-wrapper.mustache Outdated Show resolved Hide resolved
src/render-utils.ts Outdated Show resolved Hide resolved
@JHWelch
Copy link
Contributor Author

JHWelch commented Sep 26, 2023

Left some minor comments. Overall looks ok. Can you also grab the colors from GitHub I think they have a pretty nice color scheme and it would look better.

Sounds good! Will pull the github colors and update.

Updated to github colors in 987d8cb

@JHWelch JHWelch requested a review from rtfpessoa September 26, 2023 23:09
@rtfpessoa rtfpessoa merged commit b04a400 into rtfpessoa:master Sep 29, 2023
4 of 5 checks passed
@JHWelch JHWelch deleted the add-dark-mode branch September 29, 2023 23:21
@akwiatek akwiatek mentioned this pull request Oct 2, 2023
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.

Add support for dark mode
2 participants