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

Use display-color-cells to enable 24-bit color terminal support #64

Closed
wants to merge 3 commits into from

Conversation

visigoth
Copy link

This change enables the use of nord-theme with emacs' new 24-bit color terminal support.

@christopherraa
Copy link

I see this PR has been here for a while. Are there someone with write access here that could do a review? It would be nice to see if this PR solves #64 as well.

@rien333
Copy link
Contributor

rien333 commented Dec 3, 2019

I see this PR has been here for a while. Are there someone with write access here that could do a review? It would be nice to see if this PR solves #64 as well.

@arcticicestudio I've been using this for about year now. More so, I even need to use this fix, given #59.

@arcticicestudio
Copy link
Contributor

arcticicestudio commented Dec 3, 2019

Sorry for the enormous delays, but "real life" keeps me busy almost ~18h a day 😟
Like described in #59 (comment), I heavily rely on the community for this port project since I'm not a Emacs user and I resist to learn the language of syntactically arranged brace characters (Lisp) 😝

Anyway, I promise to review, reproduce and test this PR no later than Saturday afternoon. I've seen a lot of forks of this repository only for this fix and many users approved the problems was resolved using the changes from this PR so I guess it'll work the way it should without breaking any other users setup.

Copy link
Contributor

@arcticicestudio arcticicestudio left a comment

Choose a reason for hiding this comment

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

Hi @visigoth 👋, thanks a lot for your contribution 👍

Sorry for the large delay, but my free time has been really limited the last year.
Based on the feedback and test in #59 your PR seem to partially fix the loading problem.

Anyway, before merging please see the review comments I've added so we can get this fix into the next release version.

@@ -450,6 +454,11 @@
`(web-mode-variable-name-face ((,class (:foreground ,nord-variable))))

;; +--- UI ---+

;; > ace-jump-mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this additional style block and maybe submit a new PR if you'd like to add support for the package to Nord.

@@ -480,6 +489,19 @@
`(diff-hl-change ((,class (:background ,nord13))))
`(diff-hl-insert ((,class (:background ,nord14))))
`(diff-hl-delete ((,class (:background ,nord11))))

;; > diredp
Copy link
Contributor

Choose a reason for hiding this comment

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

Same like above: Please also remove this additional style block.

@arcticicestudio
Copy link
Contributor

@visigoth I can also go ahead and rebase your PR on develop and remove both unrelated code blocks if you like, just let me know so we can get this finally merged 😄

@arcticicestudio
Copy link
Contributor

Since the source branch of this PR is not a feature branch but the default branch visigoth/nord-emacs@develop there's no way for me to update the required PR changes. I'll create a new branch based on this PR and open a new PR to superseed this one.

@arcticicestudio
Copy link
Contributor

Superseded by #88.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants