Skip to content

Make Dark Mode Automatic#178

Merged
mtlynch merged 1 commit into
mtlynch:masterfrom
Kangie:auto-dark-mode
Oct 7, 2023
Merged

Make Dark Mode Automatic#178
mtlynch merged 1 commit into
mtlynch:masterfrom
Kangie:auto-dark-mode

Conversation

@Kangie

@Kangie Kangie commented Sep 27, 2023

Copy link
Copy Markdown
Contributor

Use @media (prefers-color-scheme: to determine which CSS to load based on the browser's preference.

This change consolidates all CSS into style.css and removes the manual 'Dark Mode' toggle.

Also grabbed 'development' flavours of both Prism CSS themes to make them consistent and updated Prism to 1.29.

Closes #175

@Kangie Kangie force-pushed the auto-dark-mode branch 3 times, most recently from 35aa9c1 to 6eea53e Compare September 27, 2023 08:11
@mtlynch

mtlynch commented Sep 27, 2023

Copy link
Copy Markdown
Owner

Thanks for contributing this!

Would it be possible to narrow the scope down to just what's necessary for automatic dark mode? So we'd revert the changes to emberstack/sftp and Prism because they're outside of what's necessary for the change.

I'd also like to keep the third-party code in the third-party dir, as mixing it in with first-party (LogPaste-specific) code makes it hard to do version updates in the future.

"How to Make Your Code Reviewer Fall in Love with You" also contains some tips about how to make the code contribution process work smoothly.

@Kangie

Kangie commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

It might be possible to use @import directives to directly include the relevant CSS; I'll have a play.

I'm going to need to make changes to the Prism code anyway; one version is minified and the other is not - I may as well retain the upgrade in a new commit.

I won't revert the emberstack change - that fixes CI breakage. As long as you don't squash commits when you merge the PR each of these should be an atomic change.

@mtlynch

mtlynch commented Sep 27, 2023

Copy link
Copy Markdown
Owner

I won't revert the emberstack change - that fixes CI breakage. As long as you don't squash commits when you merge the PR each of these should be an atomic change.

Can you pull this into a separate PR and make that change first?

Keep in mind, this PR will be squashed before merging, so it needs to merge cleanly with that in mind.

@Kangie

Kangie commented Sep 28, 2023

Copy link
Copy Markdown
Contributor Author

Requested changes made; Prism still updated because I'd have had to dig out an older version to make both consistent.

Keep in mind, this PR will be squashed before merging,

Why squash a PR with atomic commits? I could understand if the commits were:

  • Do thing
  • Fix bug in do_thing
  • make CI happy

I rebased and force pushed any CI/CD catches, so squashing this PR before I split it for you seems like it's being done for opaque reasons and has has added an additional 200% overhead to a simple change.

@Kangie

Kangie commented Sep 28, 2023

Copy link
Copy Markdown
Contributor Author

Also grabbed 'development' flavours of both Prism CSS themes to make them consistent

The old state was one minified and one 'development'; for consistency and ease of development / tinkering they're all 'development' flavours now, but it we need to save a few K per request maybe adding a CSS minifying step to the release-generating CI is the best solution.

It's beyond the scope of this PR regardless!

@mtlynch

mtlynch commented Sep 29, 2023

Copy link
Copy Markdown
Owner

Keep in mind, this PR will be squashed before merging,

Why squash a PR with atomic commits? I could understand if the commits were:

  • Do thing
  • Fix bug in do_thing
  • make CI happy

I rebased and force pushed any CI/CD catches, so squashing this PR before I split it for you seems like it's being done for opaque reasons and has has added an additional 200% overhead to a simple change.

I understand that you have your own workflow that's comfortable for you.

I maintain several open-source projects, and I do it just as a hobby, so I arrange workflows that optimize for my efficiency as the maintainer.

I appreciate and value contributions like yours, but it's a nontrivial increase in my time to adapt the PR review workflow for each individual contributor.

All my repos are exclusively squash and merge because that yields a consistent result regardless of the contributor's commit hygiene.

Force pushing actually makes the reviews harder because I can't compare the current version to the last version I reviewed, so I've now disabled them for this repo.

@mtlynch mtlynch left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for splitting it up! This is much easier to review.

Comment thread static/css/style.css Outdated
Use `@media (prefers-color-scheme:` to  determine which CSS to load
based on the browser's preference.

This change removes the manual 'Dark Mode' toggle.
@Kangie Kangie requested a review from mtlynch October 7, 2023 01:54

@mtlynch mtlynch left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

LGTM, thanks!

@mtlynch mtlynch merged commit 10006fd into mtlynch:master Oct 7, 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.

[Feature] Automatic light/dark theme based on system theme

2 participants