bug#21898: scss-mode font-lock face for variables

bug#21898: scss-mode font-lock face for variables

[Jackson Hamilton]

[-- Attachment #1.1: Type: text/plain, Size: 747 bytes --]

I'd like to propose the following change to the scss-mode on master: Use
font-lock-constant-face for SCSS variables.

This may not seem intuitive from a naming perspective, but
font-lock-variable-name-face is already used for CSS properties. That makes
it harder to distinguish between properties and variables.

AFAIK, Sass doesn't even have constants, so I don't see much harm in using
this face. It'd be a less dramatic change for those who have grown used to
variable coloring for CSS properties.

I guess the alternative would be to inherit the property face from
something else, to free up the face for real variables. But then what do we
use for properties? (Inheriting from nothing doesn't look good IMO.)

Attached is the proposed patch.

[-- Attachment #1.2: Type: text/html, Size: 885 bytes --]

[-- Attachment #2: 0001-Use-font-lock-constant-face-for-scss-variables.patch --]
[-- Type: text/x-patch, Size: 959 bytes --]

From 72fb4670228324b59531a19409618277b62ad230 Mon Sep 17 00:00:00 2001
From: Jackson Ray Hamilton <jackson@jacksonrayhamilton.com>
Date: Thu, 12 Nov 2015 22:21:38 -0800
Subject: [PATCH] Use `font-lock-constant-face' for scss variables

* css-mode.el (scss-font-lock-keywords): Change face.
---
 lisp/textmodes/css-mode.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 3e84b43..e35e6cb 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -525,7 +525,7 @@ pseudo-classes, and at-rules."
     st))
 
 (defvar scss-font-lock-keywords
-  (append `((,(concat "$" css-ident-re) (0 font-lock-variable-name-face)))
+  (append `((,(concat "$" css-ident-re) (0 font-lock-constant-face)))
           (css--font-lock-keywords 'sassy)
           `((,(concat "@mixin[ \t]+\\(" css-ident-re "\\)[ \t]*(")
              (1 font-lock-function-name-face)))))
-- 
2.6.3

bug#21898: scss-mode font-lock face for variables

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

Hello, Jackson!

Thanks for the report. I never noticed this issue myself, since the
theme I'm using (Leuven) styles the css-property face. We should
definitely fix it.

Like you say, two possible solutions are:

1. Highlight variables with font-lock-constant-face instead of
   font-lock-variable-name-face. A downside is that this is backwards
   wrt. the intended meaning of the faces, so users will see variables
   highlighted in a face that's usually used for constants. An upside is
   that it will probably look nice with existing themes, since they are
   likely to already style font-lock-variable-name-face.

2. Change the css-property face. It doesn't mean it has to inherit from
   another one of the standard Font Lock faces, we could also just
   change the default foreground color, for instance. A downside of this
   approach is that users may be startled that the face that they were
   used to changed, but for themes that already style the css-property,
   everything will be like before. The upside is that
   font-lock-variable-name-face remains used for variables, like it's
   intended to.

I'm not sure which solution is best.

Either way, we should also add a defface for variables and use it for
Sass variables (and also CSS variables later on).

-- Simen

On Fri, Nov 13, 2015 at 7:37 AM, Jackson Hamilton 
<jackson@jacksonrayhamilton.com> wrote:
> I'd like to propose the following change to the scss-mode on master: 
> Use font-lock-constant-face for SCSS variables.
> 
> This may not seem intuitive from a naming perspective, but 
> font-lock-variable-name-face is already used for CSS properties. That 
> makes it harder to distinguish between properties and variables.
> 
> AFAIK, Sass doesn't even have constants, so I don't see much harm in 
> using this face. It'd be a less dramatic change for those who have 
> grown used to variable coloring for CSS properties.
> 
> I guess the alternative would be to inherit the property face from 
> something else, to free up the face for real variables. But then what 
> do we use for properties? (Inheriting from nothing doesn't look good 
> IMO.)
> 
> Attached is the proposed patch.

[-- Attachment #2: Type: text/html, Size: 2761 bytes --]

bug#21898: scss-mode font-lock face for variables

[Simen Heggestøyl]

Maybe a solution could be to let the `css-property' face inherit from
`font-lock-keyword-face' instead of `font-lock-variable-name-face'? That
would let `font-lock-variable-name-face' stay reserved for CSS/Sass
variables, while being distinguishable from CSS properties by default.

-- Simen

bug#21898: scss-mode font-lock face for variables

[Jackson Ray Hamilton]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

Looks fine to me in all the default themes and the top 10 themes here: https://emacsthemes.com/charts/all-time.html

> On July 29, 2017 at 9:46 AM Simen Heggestøyl <simenheg@gmail.com> wrote:
>
>
> Maybe a solution could be to let the `css-property' face inherit from
> `font-lock-keyword-face' instead of `font-lock-variable-name-face'? That
> would let `font-lock-variable-name-face' stay reserved for CSS/Sass
> variables, while being distinguishable from CSS properties by default.
>
> -- Simen
>

[-- Attachment #2: Type: text/html, Size: 695 bytes --]

bug#21898: scss-mode font-lock face for variables

[Simen Heggestøyl]

On Sat, Jul 29, 2017 at 7:24 PM, Jackson Ray Hamilton 
<jackson@jacksonrayhamilton.com> wrote:
> Looks fine to me in all the default themes and the top 10 themes 
> here: https://emacsthemes.com/charts/all-time.html

Thanks for checking. It looks good to me in all the built-in themes as
well. I've installed the change in master as
4219240e1df6abbd842f4474fe7862f341cc355a.

-- Simen