Re: ELPA: New package: nano-theme

[Philip Kaludercic <philipk@posteo.net>]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> Dear all,
>
> I would like to submit a new package to ELPA which is a theme based on
> Material colors (https://material.io/) for the light version and Nord
> colors (https://www.nordtheme.com/) for the dark version. The theme
> itself is made of (only) 6 different faces and the rationale is here:
> https://arxiv.org/abs/2008.06030
>
> The theme is hosted at https://github.com/rougier/nano-theme and the
> README displays what it looks like. Note that the theme exploits face
> inheritance a lot and Eli explained me that it might degrade
> performances. I don't know if this needs to be fixed.

I see that a lot of faces inherit from nano-default, nano-strong,
nano-faded, etc. Wouldn't it make more sense to inherit from built-in
faces and then let default, bold, shadow, etc. inherit from these faces.

Furthermore, I wonder why you define the commands nano-light and
nano-dark, instead of two themes, nano-light and nano-dark (along the
same lines of what modus-themes currently does). You could also turn
nano-setup into a custom theme, so that the user can easily enable and
disable it.

A few more points:

- Should the font really be part of the face? I think it makes sense to
  recommend a few fonts, but it is unconventional to add it as part of a
  theme.
- When you are only using one branch of a if expression, prefer
  when. Instead of (if (not ...) ...), prefer unless.
- Lines 363ff. seem to be indented unconventionally. Maybe add a
  .dir-locals.el to make sure everyone is using the same whitespace
  configuration.

Also, as you depend on at least 27.1, you probably don't have to check
if custom-theme-load-path is defined. The same applies to tooltip-mode,
scroll-bar-mode and tool-bar-mode. What you are doing looks more like
configuration code, where you want to make sure that a .emacs works on
older versions of Emacs.

> Best,
> Nicolas
>
>

-- 
	Philip Kaludercic