GNU bug report logs - #21898
scss-mode font-lock face for variables

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 21898 in the body.
You can then email your comments to 21898 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
To: bug-gnu-emacs <at> gnu.org
Subject: scss-mode font-lock face for variables
Date: Thu, 12 Nov 2015 22:37:57 -0800

[Message part 1 (text/plain, inline)]

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.

[Message part 2 (text/html, inline)]

[0001-Use-font-lock-constant-face-for-scss-variables.patch (text/x-patch, attachment)]

Message #8 received at 21898 <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Cc: 21898 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#21898: scss-mode font-lock face for variables
Date: Fri, 13 Nov 2015 21:29:45 +0100

[Message part 1 (text/plain, inline)]

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 <at> 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.

[Message part 2 (text/html, inline)]

Message #11 received at 21898 <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Jackson Hamilton <jackson <at> jacksonrayhamilton.com>
Cc: 21898 <at> debbugs.gnu.org
Subject: Re: bug#21898: scss-mode font-lock face for variables
Date: Sat, 29 Jul 2017 18:46:07 +0200

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

Message #14 received at 21898 <at> debbugs.gnu.org (full text, mbox):

From: Jackson Ray Hamilton <jackson <at> jacksonrayhamilton.com>
To: Simen Heggestøyl <simenheg <at> gmail.com>
Cc: 21898 <at> debbugs.gnu.org
Subject: Re: bug#21898: scss-mode font-lock face for variables
Date: Sat, 29 Jul 2017 10:24:48 -0700 (PDT)

[Message part 1 (text/plain, inline)]

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 <at> 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
>

[Message part 2 (text/html, inline)]

Message #19 received at 21898-done <at> debbugs.gnu.org (full text, mbox):

From: Simen Heggestøyl <simenheg <at> gmail.com>
To: Jackson Ray Hamilton <jackson <at> jacksonrayhamilton.com>
Cc: 21898-done <at> debbugs.gnu.org
Subject: Re: bug#21898: scss-mode font-lock face for variables
Date: Sun, 30 Jul 2017 11:33:56 +0200

On Sat, Jul 29, 2017 at 7:24 PM, Jackson Ray Hamilton 
<jackson <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.