Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Dark theme #160

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Dark theme #160

wants to merge 1 commit into from

Conversation

literalpie
Copy link

I welcome feedback on anything, specifically:

  • Position and style of theme picker
  • The use of TypeScript - I chose it because I'm familiar and like type safety, but I can easily remove it
  • Fetching the JS from Swift. Based on conversations in other threads, it sounds like you have security concerns with the approach I'm using. If the SPM 5.3 resources feature will work for this, maybe it's worth waiting for that release before merging.

Hope you find this helpful!

@mattt
Copy link
Contributor

mattt commented Aug 3, 2020

Nice work, @literalpie! I look forward to checking this out in the next day or two. In the meantime, and for posterity, would you mind sharing a few screenshots here?

@literalpie
Copy link
Author

Sure, here you go!
Screen Shot 2020-08-03 at 6 11 15 PM
Screen Shot 2020-08-03 at 6 11 31 PM

@mattt mattt added design Functionality with a design component enhancement New feature or request labels Aug 4, 2020
@literalpie
Copy link
Author

literalpie commented Sep 20, 2020

I've added changes to make use of SPM resource bundling allowed in 5.3. It references an in-progress PR to update SwiftSyntaxHighlighter, so this should be considered WIP until at least that is merged, and probably a little longer if we want to wait before requiring 5.3.

If the update to 5.3 should be it's own PR, let me know and I can split it out, or anyone can use my changes as a starting point.

@mattt mattt added this to the 1.0.0-beta.6 milestone Sep 27, 2020
@literalpie literalpie force-pushed the dark-theme branch 2 times, most recently from ec8e7f6 to d21a0fd Compare October 17, 2020 13:59
@kaishin
Copy link
Contributor

kaishin commented Oct 17, 2020

@literalpie Really nice work! Glad this is on track.

I was wondering, was omitting the color for main a deliberate decision?

screenshot-2020-10-17-2

@literalpie
Copy link
Author

@kaishin There were already dark mode colors in the css file and I did not make any changes. Notice here that system-background and system-grouped-background are the same in dar mode, but different in light mode. I'm not sure what the reason for this is.

@kaishin
Copy link
Contributor

kaishin commented Oct 17, 2020

@literalpie Right, I noticed after my comment. In that case that could be changed later.

@mattt Thoughts? I can do a pass on the colors after this PR is merged if you wouldn't mind.

@literalpie
Copy link
Author

Matt, I've updated with support for both pre and post-5.3, the build is passing, and I have squashed my changes into one commit. I've also added logic to gracefully fall back when color-scheme queries are not supported, or when CSS custom properties are not supported.

Having made these changes, as far as I know, this is ready to merge. Let me know if you have any suggestions.

@literalpie
Copy link
Author

Hi @mattt I'm still interested in getting this merged. Is now a good time to rebase, or are there other changes you are planning to get done before this is merged?

I see that you've moved the CSS from being a bundled file to a string in a Swift file. Should I do something similar with the JS generated here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design Functionality with a design component enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants