metonym / svelte-highlight

Syntax Highlighting for Svelte using highlight.js
https://svhe.onrender.com
MIT License
262 stars 14 forks source link

[BREAKING] Resolving styling issues + causes #173

Closed WillsterJohnson closed 3 years ago

WillsterJohnson commented 3 years ago

Breaking Change: The .hljs class is now applied to the <code /> and not the <pre /> in each Highlight component.

Closes #172

I didn't have as much time to work on this as I thought this week, so this one took a little longer than I had hoped.

As well as the busy week, bug 4 had me scratching my head for some time. In short, the issues caused by the fix to bug 1 mean that langtags would no longer receive the correct text color from highlight.js. I tested various workarounds, however they all felt far too hacky, one of them included modifying highlight.js's styles in a way that resulted in styles leaking out of scope. The solution I landed on was the documentation update.

Bug 1 - Mismatched classes

Summary: the use of the .hljs class deviates from the intended use from highlight.js .

As to not require a major version, I suggest simply adding the class to the <code /> as well as the <pre /> .

In implementing the fix per #172, I caused a breaking change by removing .hljs class from <pre /> and adding it to <code />

Implementation

Breaking change:

Implementation Issues

Drastically highlights bug #4

Bug 2 - Over Padded "langtags"

Summary: a small mistake was made when implementing langtags, and the padding was too large.

I suggest changing the padding by the ~4px it is off by.

In implementing the fix per #172, I decreased the padding on langtags to 1em .

Implementation

Non-breaking change:

Implementation Issues

Highlights bug #4

Bug 3 - Mismatched Classes (cont.)

Summary: the css preprocessing done on ScopedStyle components in the /demo project causes some issues in the resulting CSS.

I suggest modifying one of the replacements to fix both of the issues.

In implementing the fix per #172, I modified the RegEx replacements to account for changes from bug #1.

Implementation

Non-breaking change:

Implementation Issues

Highlights bug #4

Bug 4 - Demo Project CSS

Summary: fixing the previous bugs has caused the /demo project's CSS to become partly outdated, requireing a refresh.

I suggest moving all styles from .hljs code to pre.hljs and removing padding from pre.hljs .

In implementing the fix per #172, I merged some classes in the app.css file as well as added styles to demo langtags correctly in the documentation.

Implementation

Non-breaking change:

Implementation Issues

Nothing observed, if issues do arise they will not effect users' code as the fix is scoped only to the /demo project.


Documentation change: In order to prevent hacky code, I'm simply recommending that users who want to take advantage of langtags also use the exposed custom properties. I'm unsure if this is a breaking change, however the fix for bug 1 requires a major version change either way.

This will likely be the last contribution from me, at least for a little while. It's been a lot of fun working on the earlier feature and these fixes, and quite refreshing to read through the code and see your style of programming.

metonym commented 3 years ago

@willster277 In reading the "Language Tags" docs, the first code snippet has langtag enabled; it should also be enabled in the source code to showcase the default behavior: https://github.com/willster277/svelte-highlight/blob/master/demo/routes/index.svelte#L252

WillsterJohnson commented 3 years ago

@metonym I've added this, as well as given it a text color for readability on the highlight.js theme being used

metonym commented 3 years ago

Released in v4.0.0