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

[BREAKING] Resolving styling issues + causes #173

Merged
merged 8 commits into from
Sep 16, 2021
Merged

[BREAKING] Resolving styling issues + causes #173

merged 8 commits into from
Sep 16, 2021

Conversation

WillsterJohnson
Copy link
Contributor

@WillsterJohnson WillsterJohnson commented Sep 16, 2021

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

  • Run npm run format before commit
  • Test features for bugs in /demo
  • Test features for bugs in external project
  • No breaking changes

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:

  • the .hljs class is now applied to the <code /> and not the <pre /> in each Highlight component.
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:

  • the padding on the pre.langtag::after in each of the Highlight components is set to 1em to match the padding of the pre code.hljs .
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:

  • The RegEx replacements have been changed to first apply the scope to all selectors, then apply a modification to rules that don't start .hljs (rules 1 and 2).
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:

  • The app.css file has been modified to merge .hljs and .hljs code, remove .hljs from the pre.hljs selector, and apply styles to pre[data-language="css"]
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 metonym self-requested a review September 16, 2021 17:32
Copy link
Owner

@metonym metonym left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful work – thank you for taking the time to also update the docs. See my comments

Co-authored-by: Eric Liu <ericyl.us@gmail.com>
@metonym
Copy link
Owner

metonym commented Sep 16, 2021

@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
Copy link
Contributor Author

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

@metonym metonym self-requested a review September 16, 2021 19:02
@metonym metonym merged commit ead430a into metonym:master Sep 16, 2021
@metonym
Copy link
Owner

metonym commented Sep 16, 2021

Released in v4.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Multiple issues regarding styling (linked to v3.4.0)
2 participants