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

Syntax themes: .pl-s/.pl-s1 description and usage no longer match #256

Open
auscompgeek opened this issue Sep 22, 2015 · 28 comments
Open

Syntax themes: .pl-s/.pl-s1 description and usage no longer match #256

auscompgeek opened this issue Sep 22, 2015 · 28 comments
Labels
bug 🐛 Something isn't working github :octocat:

Comments

@auscompgeek
Copy link
Contributor

The syntax highlighting themes template describes .pl-s as support. However, from what I can see, it should actually be string. This has caused strings to be highlighted as the wrong colour in rendered code views; for example, the Tomorrow Night themes highlight strings as purple rather than green.

@silverwind
Copy link
Member

Got a example?

@auscompgeek
Copy link
Contributor Author

I guess this one is simple enough.

print('This here is a string.')

@Mottie
Copy link
Member

Mottie commented Sep 22, 2015

If you look at this demo, it looks like most of the examples are highlighting strings as green. The only time I noticed the string is not green are in the HTTP, JSON and Nginx examples.

If you want to submit a custom theme, I would be happy to include it.

@auscompgeek
Copy link
Contributor Author

This isn't an issue with the themes themselves, rather, it's an issue with selecting elements incorrectly.

@Mottie
Copy link
Member

Mottie commented Sep 22, 2015

GitHub uses a custom syntax highlighter so we don't have control over that... To get the comments for the class names, I had to email them directly and that's how they labeled that selector (ref).

I'm not saying we're not going to change anything. I just don't see where the purple strings are showing up and what changes you are requesting. Maybe submit a pull request so we have a better idea?

@Mottie
Copy link
Member

Mottie commented Sep 22, 2015

Hmm, ok I actually changed my theme to Tomorrow Night and I am seeing the purple strings now... odd that the demo is different.

@auscompgeek
Copy link
Contributor Author

Guess GitHub might've changed the classes on us. It wouldn't be the first time. Could we contact them and see what else has changed since then? .pl-s is almost definitely string. Here's a couple more test cases:

JS:

console.log("Hello, World!");

bash:

echo "Say $(echo 'Hello World!')."

@Mottie
Copy link
Member

Mottie commented Sep 22, 2015

Yeah, if you look at #197, you'll see they used to have strings named as .pl-s1...

sigh I don't have a lot of time to work on this today, but I'll start with updating the index.html demo file and go from there.

@Mottie
Copy link
Member

Mottie commented Sep 22, 2015

Another change I noticed while updating the demo is that the class name of the language has changed. It used to add "highlight-css" after syntax highlighting was applied, now it throws a "source" into the class name "highlight-source-css" - this applies to all highlighted languages.

Also, so far, the following language class names were changed:

  • highlight-text-html-basic - used to be highlight-html
  • highlight-text-html-php - used to be highlight-php
  • highlight-source-c++ - used to be highlight-cpp
  • highlight-source-shell - used to be highlight-bash
  • highlight-text-tex - used to be highlight-latex
  • highlight-source-fortran-modern - used to be highlight-fortran

@auscompgeek
Copy link
Contributor Author

Sounds like the highlight- class names are now based on the scope names listed in grammars.yml and/or lib/linguist/languages.yml in github/linguist.

@auscompgeek
Copy link
Contributor Author

Hmm. I found a case where .pl-s1 is used now. Shell sessions:

$ echo 'Hello, World!'
Hello, World!
$ cat file.txt
This is a random file.

Here is the grammar file for it. I'm guessing .pl-s1 might be a source inclusion or something?

@auscompgeek
Copy link
Contributor Author

Guess it is. Here's some more .pl-s1, this time Literate Haskell:

Test 1:

\begin{code}
module Main where
main = putStrLn "Hello, World!"
\end{code}

Test 2:

> module Main where
>
> main = putStrLn "Hello, World!"

.pl-s1 isn't styled by GitHub's CSS at all, from what I can tell.

@auscompgeek auscompgeek changed the title .pl-s description and usage don't match Syntax themes: .pl-s/.pl-s1 description and usage no longer match Oct 3, 2015
@auscompgeek
Copy link
Contributor Author

FYI, here's another old reference mapping of scopes to CSS class names.

@Mottie
Copy link
Member

Mottie commented Oct 15, 2015

Thanks for the update... I'll try to find some time this weekend to work on a fix.

@auscompgeek
Copy link
Contributor Author

I found sindresorhus/github-markdown-css today. The commit history might be interesting/useful.

@Mottie
Copy link
Member

Mottie commented Oct 18, 2015

Hmm, well after looking at everything, the only thing I can come up with is to just set .pl-s to have the same color as .pl-s1. Any objections?

@silverwind
Copy link
Member

I wouldn't do that. pl-s is blue in GH's sheet, while pl-s1 is uncolored (renders as #333). Maybe we should just make pl-s1 the same color as the theme's body color.

@auscompgeek
Copy link
Contributor Author

I concur with @silverwind. I'm not particularly fond of random source code being highlighted like strings.

However, if we do this, we should also be aware of the fact that shell session output (.pl-mo) won't be easily distinguishable like it is with GH's CSS.

After some investigation, there is one place where .pl-s1 is specifically styled by GH — in template strings, a la Ruby:

puts "#{x} #{y.infinite ? 'TOO LARGE' : y}"

or JavaScript:

console.log(`Hello, ${name}. This is the ${console}. Testing raw strings:`,
            String.raw`a\b\n`);

@auscompgeek
Copy link
Contributor Author

Looks like sindresorhus/github-markdown-css@1f877b6 is when the .pl-s and .pl-s1 change happened, so I guess it's been in this state since at least April. Scary.

@auscompgeek
Copy link
Contributor Author

At this point in time, I would suggest changing .pl-s1 to .pl-s, and .pl-s2 to .pl-s1.

@silverwind silverwind added the bug 🐛 Something isn't working label Feb 4, 2016
@Mottie Mottie modified the milestone: testing milestone Jul 6, 2016
@Mottie
Copy link
Member

Mottie commented Oct 1, 2016

I just updated the github/_template.css file with the comments (element names) I found in the github-dark.css file created by GitHub. The new comments were added above the definition, so any in-line comments may be attached to unnecessary definitions.

It does not include a .pl-s2 class at all. In fact, it seems that the github-dark.css file is missing a bunch of definitions that we have included in the template. I added the GitHub-dark theme to the demo and it seems to work fine without those extra definitions.

Maybe, after I get back from my vacation, we can clean up the syntax themes by removing those extra definitions.

@the-j0k3r
Copy link
Member

Thats some long vacation :)

@Mottie
Copy link
Member

Mottie commented Jun 21, 2018

Perpetual vacation.

@the-j0k3r
Copy link
Member

Thats called death :)

@the-j0k3r
Copy link
Member

Is this something we can fix or what?

@Mottie
Copy link
Member

Mottie commented Jul 17, 2018

@auscompgeek
Copy link
Contributor Author

I think the sensible thing to do forward would be to use the primer syntax theme output linked above (which, side note, was updated to include a carriage-return scope) as a template for all the syntax themes.

Should also go through and remove rules for scopes that have been removed too.

@the-j0k3r
Copy link
Member

the-j0k3r commented Jul 18, 2019

Ive updated the Github syntax theme a while back now, its needing a cleanup fro unused values, help welcome.

In fact All styles need some cleanup and consistency fixes besides what I already done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working github :octocat:
Projects
None yet
Development

No branches or pull requests

4 participants