-
Notifications
You must be signed in to change notification settings - Fork 498
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
Increase specificity of selectors for plugin overrides #141
Conversation
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: prism-themes/themes/prism-cb.css Lines 169 to 171 in e6cf989
Changing its value moves it as such:
The original stylesheet does not declare The following code shifts the numbers to the left and adds a 3px solid yellow border: prism-themes/themes/prism-cb.css Lines 173 to 176 in e6cf989
I think the selector is okay, at least for However, I don't know about
(Disabling the I'm not super sure what steps to take. My thoughts are:
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? |
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) prism-themes/themes/prism-cb.css Lines 127 to 142 in e6cf989
prism-themes/themes/prism-xonokai.css Lines 148 to 164 in e6cf989
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? |
Holy! Thank you for doing all that work @hoonweiting!
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.
The Line numbers plugin supports up to 999 lines. That
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. |
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? |
That would be great! Thank you!
Sorry that we don't have something like that already. But yes, something in that direction would be very much appreciated. |
By the way I found this: prism-themes/themes/prism-shades-of-purple.css Lines 195 to 200 in e6cf989
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): Here's the same sample, but with a different theme: 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.) |
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. |
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 :) |
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 ( |
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 Do you want to make a PR? |
Sure thing! |
Huge thank you @hoonweiting! |
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:
prism-themes/themes/prism-cb.css
Lines 168 to 176 in e6cf989