PrismJS / prism-themes

A wider selection of Prism themes
MIT License
1.4k stars 504 forks source link

Increase specificity of selectors for plugin overrides #141

Closed hoonweiting closed 3 years ago

hoonweiting commented 3 years ago

This PR came about as a result of this comment, which pointed out that we do not necessarily have control over the ordering of stylesheets, and hence selectors for plugin overrides need to have higher specificity values than the those in the plugins' default stylesheets.

I need to play around with this section further because I don't yet know what it does: https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-cb.css#L168-L176

hoonweiting commented 3 years ago

Hey I'm not sure if I need to increase the specificity of the selectors I pointed out in my previous comment. In fact, I'm wondering if these properties should be removed/modified instead.

The following code sets the margin of the line numbers 'column' to 0: https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-cb.css#L169-L171

Changing its value moves it as such: margin: 0; margin: 0.5em; margin: 0 0.5em;
image image image

The original stylesheet does not declare margin for .line-numbers-rows (with/without ancestors) (and no other theme declares it too), so a margin of 0 is the default. I don't see why it has to be declared at all?


The following code shifts the numbers to the left and adds a 3px solid yellow border: https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-cb.css#L173-L176

I think the selector is okay, at least for border-right, because perhaps the creator wanted a two-toned border.

However, I don't know about padding-right. I guess it's fine if we're just looking at single digits, but double digits is a bit tight, and triple digits are not even aligned with the double digits. Changing the value of padding-right moves it as such: padding-right: 10px; padding-right: 5px; padding-right: 0;
image image image image image image

(Disabling the margin property from earlier does not affect the above either, for the record.)


I'm not super sure what steps to take. My thoughts are:

  1. Completely removing .line-numbers { margin: 0; }, and
  2. Reducing padding-right of .line-numbers-rows span or just removing it completely, and possibly
  3. Increasing the specificity of .line-numbers-rows span to be consistent.

I'm not super sure about the second point, because maybe the creator optimised it for <100 lines (or line numbers in the range of [-9, 99])?

What are your thoughts?

hoonweiting commented 3 years ago

In addition, I also came across quite a few themes that re-declared the exact same selectors/properties as the plugins' default stylesheets. For example:

https://github.com/PrismJS/prism/blob/0ff371bb4775a131634f47d0fe85794c547232f9/plugins/line-highlight/prism-line-highlight.css#L1-L20 (unfortunately the code block does not want to be displayed) https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-cb.css#L127-L142 https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-xonokai.css#L148-L164

I mean, some of the properties are different for good reason, such as the background colour, but the other properties have no need to be declared?

RunDevelopment commented 3 years ago

Holy! Thank you for doing all that work @hoonweiting!

I'm not super sure what steps to take. My thoughts are:

I agree with all of it.

I think it's ok to just remove it. The plugin might have had slightly different styles in the past.

I'm not super sure about the second point

The Line numbers plugin supports up to 999 lines. That padding-right brings it down to only up to 99 lines which is not acceptable.

I mean, some of the properties are different for good reason, such as the background colour, but the other properties have no need to be declared?

That was probably the authors just copying the plugin styles and making some adjustments. Not only does it have a specificity problem but it's also bad for forward compatibility.

We may fix bugs in those plugins in the future that involve changing the properties of those rules. Those themes will bring those fixed bugs right back.

hoonweiting commented 3 years ago

Thank you! :) I'll work on it now!

Also, some of the themes declare selectors for some plugins, but with selectors that aren't the same as those in the default stylesheet. I had ignored them since they didn't overlap, but now writing this comment, I realised I didn't actually calculate their specificity so that might be a problem, oops. Anyway, I wanted to say, I'm thinking of bringing them in line with what I've been doing, so it's much more standardised and easier for maintenance moving forward. Would that be alright?

And speaking of bringing them in line, I'm thinking of creating a template stylesheet or at least a document on theme guidelines (probably in the next few weeks), since I had already poked around most of it with DevTools, haha. What's your take on this?

RunDevelopment commented 3 years ago

Would that be alright?

That would be great! Thank you!

I'm thinking of creating a template stylesheet or at least a document on theme guidelines

Sorry that we don't have something like that already. But yes, something in that direction would be very much appreciated.

hoonweiting commented 3 years ago

By the way I found this: https://github.com/PrismJS/prism-themes/blob/e6cf98987533b64f51bdbc0c902861b46e14c96c/themes/prism-shades-of-purple.css#L195-L200

What it does is...remove the line numbers of the highlighted line... For example (with lines 3-6, and 11 highlighted; line numbers plugin disabled): image

Here's the same sample, but with a different theme: image

I mean, I think the removal of the line numbers looks intentional, but I don't know if that should be the case? The author might have desired this behaviour, but without any warning given to users, I think it's a potential cause of frustration. (Of course, maybe users just use both line numbers and line highlight, so they don't encounter this issue at all!) Should I remove it, or just let it be?

(Side note: I don't know why the highlighted line is just off, but it's the same story for the themes I've checked so far (with varying degrees of being off). Not sure if the stylesheet I'm using in the rest of the page has something to do with it, but I think it is beyond the scope of this PR so I shall leave it be.)

RunDevelopment commented 3 years ago

I think it's okay to leave it for now.

The real problem is that plugins don't provide an interface for themes to change them. If we had, then all these problems would disappear. Unfortunately, Prism still supports IE11 which doesn't know what CSS variables are.

Man, we really have to start working on Prism v2.

hoonweiting commented 3 years ago

Ah... I wish I could help with the interface part (as well as the other goals for V2), but it is currently beyond my skillset! But I'll be glad to help out with stylesheets or stuff that's more on the design side which I'm a little more familiar with :)

hoonweiting commented 3 years ago

Oh and one more thing, should the specificities of the selectors for plugin overrides for themes over at the main repo be increased too? There's three themes which define any overrides, though seems like only one (or two?) of them lacks the increased specificities.

Also, the repeated class that was chosen in funky (.diff-highlight) differs from the repeated class I chose (.token). Should I swap out .token for .diff-highlight instead? (As well as .rainbow-braces)

RunDevelopment commented 3 years ago

There's three themes which define any overrides, though seems like only one (or two?) of them lacks the increased specificities.

Coy: I think what Coy does is okay. It essentially does a mix between repeating classes and adding new requirements.

Funky: Which class gets repeated doesn't matter, so I think we can just leave it.

Twilight: Yeah, that definitely needs fixing. The pre[data-line] rule can even be removed since it's the same as in prism-line-highlight.css.

Do you want to make a PR?

hoonweiting commented 3 years ago

Sure thing!

RunDevelopment commented 3 years ago

Huge thank you @hoonweiting!