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

add a bunch of faces for helm, eldoc, sh, iedit, fix some issues #68

Closed
wants to merge 14 commits into from

Conversation

lewang
Copy link
Contributor

@lewang lewang commented Sep 2, 2012

No description provided.

Copy link
Owner

@sellout sellout left a comment

Choose a reason for hiding this comment

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

There are a mix of changes here, some of which fit better than others. I’m going to close the PR, considering the age, conflicts, and amount of it that I won’t merge, but I welcome a new PR with just the parts I mentioned (if you’re even still using this theme).

@@ -168,7 +168,7 @@ the \"Gen RGB\" column in solarized-definitions.el to improve them further."
(fmt-bldi `(:weight ,bold :underline nil :inverse-video nil))
(fmt-undr `(:weight normal :slant normal :underline ,underline :inverse-video nil))
(fmt-undb `(:weight ,bold :slant normal :underline ,underline :inverse-video nil))
(fmt-undi `(:weight normal :underline ,underline :inverse-video nil))
(fmt-undi `(:weight normal :slant ,italic :underline ,underline :inverse-video nil))
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately (despite the apparent obviousness of this) the upstream theme doesn’t set italic on fmt-undi.

I’ll probably publish a variant of Solarized that does the obvious thing for fmt-undi, fmt-bldi, and a couple other things that don’t match.

`((diff-added ((t (,@fg-green))))
(diff-changed ((t (,@fg-yellow))))
(diff-removed ((t (,@fg-red))))
(diff-refine-change ((t (,@fg-blue ,@bg-back))))))))
(diff-refine-change ((t (:foreground "black" ,@bg-base01))))))))
Copy link
Owner

Choose a reason for hiding this comment

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

"black" is not a color used in this theme.

I’m planning to support something like #191 soon, which will make it much easier to locally extend Solarized without having to modify the repo.

(org-level-5 ((t (,@fmt-none ,@fg-base1))))
(org-level-6 ((t (,@fmt-none ,@fg-base01))))
(org-level-7 ((t (,@fmt-none ,@fg-orange))))
(org-level-8 ((t (,@fmt-none ,@fg-violet))))
Copy link
Owner

Choose a reason for hiding this comment

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

These have since been added to the theme, but they simply :inherit (outline-*). The first three and last three match yours, but the middle two use fg-red and fg-base0.

;; mmm-mode
(mmm-code-submode-face ((t (:background nil ,@fmt-ital))))
(mmm-output-submode-face ((t (:background nil ,@fmt-undi))))
(mmm-comment-submode-face ((t (:inherit font-lock-comment-face))))
Copy link
Owner

Choose a reason for hiding this comment

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

I would probably be likely to accept these additions, with some modifications:

  • :height doesn’t get set in Solarized
  • :background nil can now be done as ,@bg-none

;; cperl -- commenting out until refined.
(cperl-hash-face ((t (,@fmt-none :background nil ,@fg-yellow))))
(cperl-nonoverridable-face ((t (:inherit font-lock-keyword-face))))
(cperl-nonoverridable-face ((t (,@fmt-none ,@fg-red))))
Copy link
Owner

Choose a reason for hiding this comment

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

I would probably accept these, too.

@sellout sellout closed this Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants