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

Refactor Dark Plus and add new maintainer #10574

Merged
merged 6 commits into from
Apr 24, 2024

Conversation

David-Else
Copy link
Contributor

@David-Else David-Else commented Apr 23, 2024

I have brought #8719 back from the dead, it was confirmed good by the previous theme maintainer @kirawi. It started out as a load of changes that mainly got rejected, and ended up as a really nice refactor making it much more readable.

I have merged in the new changes from master, and attempted to credit @saccarosium using the info from https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors in the commit details (with a temp email that might need updating if it is needed at all).

@the-mikedavis I am not sure if I need to add an accurate Co-authored-by: NAME NAME@EXAMPLE.COM in the commit to credit saccarosium, or maybe the fact I merged his closed PR and see his commits will do this automatically? If I need an email, how do I obtain the right email and do that.

@kirawi You asked me if I wanted to take over maintenance of this theme in the previous thread. I will have a go, cheers!

The only visible difference between the refactor and the previous version I can find is that the whitespace is now much better and more accurate to the original, here is VS Code:

image

and here is Helix with this PR

image

It used to be this, much too dark and in your face:

image

@the-mikedavis
Copy link
Member

Since you merged the PR you have all his commits so I would consider co-author unnecessary here.

At some point @pascalkuthe and I would like to flesh out the CONTRIBUTING.md and have a section on this: Co-authored-by is good when you want to take part of someone else's commit or sometimes if you're making a change that follows their instructions to the letter. If you can copy their full commit so that git recognizes them as the author (either via cherry-picking or merging as you have here) that's even better, but I usually don't do that if I make any edits to their commit.

But when merging this I would 'squash & merge' so that you are the author of the commit and make sure that @saccarosium is tagged as Co-authored-by. (Squash + merge just to get rid of the merge commits)

@pascalkuthe pascalkuthe merged commit 22960e0 into helix-editor:master Apr 24, 2024
6 checks passed
@David-Else David-Else deleted the dark_plus_refactor branch April 24, 2024 09:53
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
* Make dark_plus.toml more accurate to VSCode

* theme(dark_plus): make type.builtin blue

* Refactor dark_plus and add myself as new maintainer

Co-authored-by: NAME <NAME@EXAMPLE.COM>

---------

Co-authored-by: Luca Saccarola <96259932+saccarosium@users.noreply.github.com>
Co-authored-by: Luca Saccarola <github.e41mv@aleeas.com>
Co-authored-by: NAME <NAME@EXAMPLE.COM>
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
* Make dark_plus.toml more accurate to VSCode

* theme(dark_plus): make type.builtin blue

* Refactor dark_plus and add myself as new maintainer

Co-authored-by: NAME <NAME@EXAMPLE.COM>

---------

Co-authored-by: Luca Saccarola <96259932+saccarosium@users.noreply.github.com>
Co-authored-by: Luca Saccarola <github.e41mv@aleeas.com>
Co-authored-by: NAME <NAME@EXAMPLE.COM>
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
* Make dark_plus.toml more accurate to VSCode

* theme(dark_plus): make type.builtin blue

* Refactor dark_plus and add myself as new maintainer

Co-authored-by: NAME <NAME@EXAMPLE.COM>

---------

Co-authored-by: Luca Saccarola <96259932+saccarosium@users.noreply.github.com>
Co-authored-by: Luca Saccarola <github.e41mv@aleeas.com>
Co-authored-by: NAME <NAME@EXAMPLE.COM>
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.

5 participants