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

Faulty Solarized definition #124

Closed
egmontkob opened this issue Sep 30, 2019 · 4 comments · Fixed by #148
Closed

Faulty Solarized definition #124

egmontkob opened this issue Sep 30, 2019 · 4 comments · Fixed by #148
Labels
beginner-friendly Good for newcomers bug Something isn't working help wanted Extra attention is needed

Comments

@egmontkob
Copy link

The Solarized definition is faulty both in the example config, as well as on the screenshots.

The Solarized palette, including the mapping to the 16 indices is clearly defined on its homepage, under "The Values". The palette index is under the column "16".

Note that this mapping does not alter between light and dark color schemes (whereas in your config and in your screenshot it does). As demonstrated in the HTML example on the homepage, using the @mixin rebase, it's the responsibility of whoever wishes to provide a Solarized-base color experience (in that case: the web developer; in our case: a terminal-based app's developer) to use different entries for different purposes accordingly.

Neither your light nor your dark definition matches with the index assignment provided on the Solarized homepage. E.g. for index 0, you use base03 in the dark variant and base2 in the light one, whereas both should use base02 instead. Etc. This is unlikely to result in a decent experience with any Solarized application.

Note that the Solarized specification is not aware of the notion of "default foreground" and "default background", something that exists in terminals, but not on webpages. This is where you can define an overall light as well as an overall dark experience.

@cdepillabout cdepillabout added beginner-friendly Good for newcomers bug Something isn't working help wanted Extra attention is needed labels Sep 30, 2019
@cdepillabout
Copy link
Owner

@egmontkob Thanks for catching this! I guess the solarized website you are referring to is https://ethanschoonover.com/solarized/?

I don't personally use the solarized theme, so I wasn't aware it was incorrect. I believe it was added by @craigem.

If you (or anyone else) wants to send a PR fixing this, I will happily merge it in.

Also, see #94. That issue is about adding predefined color schemes exported directly from termonad (not just defined in an example file). Since it seems like multiple people have wanted this solarized color scheme, I think it would be a good candidate to export directly from the termonad config. PRs are of course welcome!

@egmontkob
Copy link
Author

I have to admit I don't use Solarized either (even though I like it a lot), and don't use Termonad either, and know very little about Haskell. I'm just randomly commenting (in fact, have reported the same problem for multiple terminals).

I placed the example file as my config but it didn't start up, and I didn't care enough to try harder. Which means I can't easily verify a candidate fix. So don't expect a PR from me, sorry.

@craigem
Copy link
Contributor

craigem commented Sep 30, 2019

I'm not sure I grok the problem here. I use both light and dark Solarized themes everyday and my experience is that they work correctly, both as documented and as implemented.

I do recall having some issues mapping Solarized as defined into Termonad so that may be why egmontkob is having some dissonance between Solarized as it's defined and as it's been implemented.

Feel free to assign this to me though and I'll have a look at it.

@cdepillabout
Copy link
Owner

Since @egmontkob doesn't use Termonad (or Solarized...?), and @craigem says that he doesn't see any problems, I'm just going to go ahead and close this issue.

@craigem Although if you want to look into this and do find a problem, feel free to post here and I'll open this issue back up.

craigem added a commit to craigem/termonad that referenced this issue May 24, 2020
While porting another theme to termonad, I discovered discrepencies that
had been mentioned previously but were not noticeable in use.

Resolves cdepillabout#124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner-friendly Good for newcomers bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants