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

diff.plus, diff.minus and diff.delta theme keys need updating for many themes for git gutters #4972

Closed
10 tasks done
David-Else opened this issue Dec 2, 2022 · 13 comments · Fixed by #4979 or #5334
Closed
10 tasks done
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements

Comments

@David-Else
Copy link
Contributor

David-Else commented Dec 2, 2022

Since git gutters landed #3890 many themes need the diff colors updating, I have taken screen shots of the ones I consider in obvious need of attention, but appreciate some changes may be subjective.

*EDIT There is also "diff.delta.moved" to consider, I missed that, I am not sure if it is needed?
*EDIT Theme authors, a new release of Helix is planned this weekend, so please make any changes you want ASAP :)

  • emacs
  • flatwhite
  • kanagawa
  • serika_light
  • acme
  • nord_light
  • hex_steel
  • hex_toxic
  • zenburn
  • doom_acario_dark

Correct Example

Here is an example of dark_plus (VS Code theme) which has been updated specifically for #3890:

dark_plus_correct

Need Attention

There follows screen shots of those that seem in need of updates, there maybe more, this is just my opinion. I have tried to add a ping to the authors:

@Yevgnen
Screenshot from 2022-12-02 14-57-48

@David-Else David-Else added the C-enhancement Category: Improvements label Dec 2, 2022
@pascalkuthe pascalkuthe added the A-theme Area: Theme and appearence related label Dec 2, 2022
@theli-ua
Copy link
Contributor

theli-ua commented Dec 2, 2022

Is there a way to style gutter symbols separately from patch/diff highlighting?

@David-Else
Copy link
Contributor Author

David-Else commented Dec 2, 2022

@theli-ua No, afraid not, these theme keys were only meant for diff gutters: #3890 (comment)

@Aethrexal
Copy link
Contributor

There we go, Doom Acario have gotten a change.

@archseer archseer reopened this Dec 3, 2022
@pascalkuthe
Copy link
Member

Thanks @David-Else for going trough the effort of putting this list together!

@xcdkz
Copy link
Contributor

xcdkz commented Dec 5, 2022

Acme and Nord Light are merged now 👍

@David-Else
Copy link
Contributor Author

Acme and Nord Light are merged now +1

Awesome! 6/10 complete... Any update from @Yevgnen @AlexanderBrevig @zetashift @VuiMuich ?

@AlexanderBrevig
Copy link
Contributor

I'll have a look soon. Should maybe be added to the linter if it's not already?

@David-Else
Copy link
Contributor Author

The linter could check the keys exists, and that they are different, but many of the problems have been caused by a bad choice of colors as there were no diff gutters for visual inspection.

@VuiMuich
Copy link
Contributor

VuiMuich commented Dec 5, 2022

Acme and Nord Light are merged now +1

Awesome! 6/10 complete... Any update from @Yevgnen @AlexanderBrevig @zetashift @VuiMuich ?

Sorry for the delayed response, have been quite offline the past couple days. I am confident to find some time tomorrow at about the same hour to push an update.

@VuiMuich
Copy link
Contributor

VuiMuich commented Dec 5, 2022

Realised it took actually less then anticipated, as the diff-tags were there and just the color choice was bad...

@AlexanderBrevig
Copy link
Contributor

I've made a PR for flatwhite #5036 and a PR for adding the diff to themelint #5037

@David-Else
Copy link
Contributor Author

@AlexanderBrevig 22.12 came out, you just missed the boat for the flatwhiteupdate getting in it. Great you updated the themelint!

@David-Else
Copy link
Contributor Author

David-Else commented Dec 28, 2022

@Yevgnen Are you able to update the emacs theme in the near future?

Could anyone else maybe fix it? It would be nice to close this issue, we are now at 9/10 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-theme Area: Theme and appearence related C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants