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

render: support dark themes #613

Merged
merged 63 commits into from
Feb 23, 2023
Merged

render: support dark themes #613

merged 63 commits into from
Feb 23, 2023

Conversation

vfosnar
Copy link
Contributor

@vfosnar vfosnar commented Jan 6, 2023

Hey, I tried to create my own dark theme, but I found out there is no way to change the background. There was also hardcoded foreground color for arrow labels so I changed it to N1 (Text).

I added colors from the Cattpuccin Mocha Mauve color scheme for testing and because it looks really nice.

Some things are still missing, Latex and code blocks render horribly, but I just wanted to share my changes. What do you all think about this?

What is working

Classes
Containers
Sequence
SQL
Arrows

And what is not AFAIK

Latex
Code

@alixander
Copy link
Collaborator

whoa very cool! can you create issues for what's blocking dark themes? they should be minor changes that i'd be happy to make to unblock @vfosnar

@vfosnar vfosnar mentioned this pull request Jan 6, 2023
10 tasks
@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 6, 2023

whoa very cool! can you create issues for what's blocking dark themes? they should be minor changes that i'd be happy to make to unblock @vfosnar

Thanks! Made the issue, think I covered everything but let me know if I missed something.

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 7, 2023

Hi, had some free time today so I tried to fix #619 and think it is working now. I also added --sketch_bg option and raised an issue #622 to discuss it. I also tried updating the tests to reflect my changes but they fail at something I didn't change AFAIK.. I would be grateful if you could take a look at them because I am not be able to @alixander

Light

light

Dark

dark

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

can you add some tests of shapes with varying fill for sketch mode to see the overlay stuff?

https://github.com/terrastruct/d2/blob/master/d2renderers/d2sketch/sketch_test.go

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 11, 2023

Will look into that

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 11, 2023

I made a demo while adding the tests:

color

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 11, 2023

Never wrote tests in Go or like unit tests in general so sorry for any misunderstanding from my side:(

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 11, 2023

Also a note for myself because I left some colors unimplemented and it looks like they really should have been implemented :D

@alixander
Copy link
Collaborator

alixander commented Jan 11, 2023

the sketched overlays look good. would it be possible to split that into its own PR or is it intertwined? i'd be ready to merge that, whereas this one might take some more review and discussion.

@vfosnar
Copy link
Contributor Author

vfosnar commented Jan 11, 2023

Sadly no, it heavily depends on the new implementation of theming. That's why it was one of the last things on my TODO

@alixander
Copy link
Collaborator

gotcha. okay, looking forward to reviewing, feel free to request when it's ready!

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

absolutely sick.

dark.mp4

bad diffs:

(probably has to do with that rx thing you left a comment on)
Screen Shot 2023-02-21 at 7 20 25 PM

Screen Shot 2023-02-21 at 7 21 13 PM

(looks like some markdown colors got rewritten)
Screen Shot 2023-02-21 at 7 21 43 PM

@alixander
Copy link
Collaborator

@bo-ku-ra is our stress-tester 😄

@bo-ku-ra
Copy link
Contributor

absolutely sick.

Is that video wrong?

@alixander
Copy link
Collaborator

@bo-ku-ra https://www.urbandictionary.com/define.php?term=sick

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

absolutely sick.

thanks!

probably has to do with that rx thing you left a comment on

that todo was meant more like "we should follow a convention but won't break if we don't" I will investigate this when I get to a computer

looks like some markdown colors got rewritten

think this was intentional cuz the hardcoded values looked meh with a dark theme

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

fixed. it was a small typo :p

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

@vfosnar awesome i think this is good to go then! can you squash your commits and then add a changelog line?

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

@alixander alright!

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

@alixander welp, it throws some error...

Merge conflict in ci/release/changelogs/next.md

can't I just update the changelog and you will merge squash it?

@alixander
Copy link
Collaborator

@vfosnar we have squash merges turned off for some reason of CI I don't remember. you don't have to update the changelog, but squashing 60 commits down would be good

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

I understand that, but when I run

git rebase -i 80892f9ff9317b709d7176a6bffbbd4ca92bf9a3

It throws

Auto-merging ci/release/changelogs/next.md
CONFLICT (content): Merge conflict in ci/release/changelogs/next.md
error: could not apply 3a5e5bd1... changelog
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 3a5e5bd1... changelog

and when I do as it says and resolve it, it throws the same error on another file

@alixander
Copy link
Collaborator

@vfosnar oh, yeah that's annoying, idk what to do when i hit those either. can you give this a shot? https://stackoverflow.com/questions/25356810/git-how-to-squash-all-commits-on-branch

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 22, 2023

Okayy, looks like I'm really bad at git. Dunno how to make this work with a fork, maybe you could try merging it yourself? https://www.baeldung.com/ops/git-squash-commits#squashing-by-merging-with-the---squash-option

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

ah it's okay we'll just merge without squashing. doing one last look though -- looks like there's a regression here.

Screen Shot 2023-02-22 at 12 19 40 PM

@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 23, 2023

ah it's okay we'll just merge without squashing

alright, sorry for that

looks like there's a regression here

fixed! I did notice this but I thought it was current behavior and needs to be fixed in the future #613 (comment)

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

giphy (3)

ty for pushing this through!

@alixander alixander merged commit 619a3ab into terrastruct:master Feb 23, 2023
@vfosnar
Copy link
Contributor Author

vfosnar commented Feb 23, 2023

thank you too for you cooperation and for this awesome project!

@nhooyr
Copy link
Contributor

nhooyr commented Feb 23, 2023

super excited for this!!! thanks @vfosnar

Comment on lines +42 to +46
case colorString[0:2] == "AB":
switch colorString[2] {
case '4', '5':
return AB4, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@vfosnar i just realized, is there a reason why 4 and 5 are aggregated here?

Copy link
Collaborator

@alixander alixander Feb 24, 2023

Choose a reason for hiding this comment

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

i'm assuming it's a mistake (even though the code here looks deliberate). fixed here: #883

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, AFAIR this code should shift any given theme color to the next one and when it was the first one, it will return itself. Thus if we have 2 colors, we return same color in both cases.

This whole shifting code was intended more like a "somewhat working" solution and I thought I left a TODO explaining it somewhere there and that a better solution may be available but would probably require adding all rn missing colors to themes and tweaking them a little.

also I think this method does the opposite in dark mode, it lightens the value, becase dark themes have flipped pallet.

reference to why I thought shifting could work:
https://github.com/terrastruct/d2/tree/master/d2themes#color-coding-example

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh shoot yeah i didn't look at the function name, this makes sense. hmmm, will think about this.

Copy link
Collaborator

Choose a reason for hiding this comment

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


// Alternative colors B
AB4 = "AB4"
AB5 = "AB4"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, no it's not.

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.

4 participants