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

(themes) Add tokyo-night-light #3482

Merged

Conversation

Vanderscycle
Copy link
Contributor

@Vanderscycle Vanderscycle commented Feb 15, 2022

Changes

added tokyo-night-light and made some corrections to comments on my previous pr. Original repo
image

checkTheme.js resultss
image

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Feb 15, 2022

How much of this is a "direct port" vs a HLJS imagining of the theme? I was going to ask you to tune that 1.96 ratio to at least a 3, but now I'm wondering how we should treat "externally" ported themes from an accessibility POV. IE, should baseline accessibility or "fidelity to the original" be more important drivers?

@highlightjs/core Any thoughts?

@Vanderscycle
Copy link
Contributor Author

@Vanderscycle Vanderscycle commented Feb 15, 2022

Its an accurate port as far as my adapting process went, but I did have to squint my eyes at certain parts of the code. I should have created this pr as a draft first.

@Vanderscycle Vanderscycle marked this pull request as draft Feb 15, 2022
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Feb 15, 2022

I think perhaps for accessibility that a pass over ALL the comment styles in all (1st party, not Base16) themes and pulling them up to a 3 or 4.5 contrast and then recommend that those who want the common "i can barely see the comments" look/feel should apply opacity of their desired choosing to hljs-comment. That way we equally support both accessibility and user choice.

I'm going to split this into a separate issue and add it to the v12 list.

@allejo
Copy link
Member

@allejo allejo commented Feb 17, 2022

I think perhaps for accessibility that a pass over ALL the comment styles in all (1st party, not Base16) themes and pulling them up to a 3 or 4.5 contrast and then recommend that those who want the common "i can barely see the comments" look/feel should apply opacity of their desired choosing to hljs-comment. That way we equally support both accessibility and user choice.

I really like the opacity idea! i.e. be accessible by default, and you have to opt-in to make it less accessible. You got my vote on this approach.

However, I agree with your statement of splitting this off into a separate thread. I feel like it'd be appropriate to have some discussion/thoughts regarding how we define an "accessible theme." WCAG is flawed in some cases and there's a new player in town: APCA (it even comes with its own npm package we can use!). So I'd want to explore these options before we make a decision.

But for my immediate response as to what to do right now for new themes: default to accessible colors, opt-into less accessible with opacity. Yes, we're porting a theme from another location and ideally, we'd want to be loyal to the original, but that doesn't mean we should shrug our shoulders and be the provider of something inaccessible if it's being introduced into our product.

tools/developer.html Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Mar 1, 2022

Please see review comments. Hope to push out a release soon, if this is ready it can be part of 11.5 :-)

@joshgoebel joshgoebel added this to the 11.5 milestone Mar 1, 2022
@Vanderscycle
Copy link
Contributor Author

@Vanderscycle Vanderscycle commented Mar 1, 2022

@joshgoebel I will work on it today, sorry that my pr is taking forever.

@Vanderscycle Vanderscycle marked this pull request as ready for review Mar 1, 2022
@joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Mar 2, 2022

@Vanderscycle No problem, glad to see it getting finished up!

@joshgoebel joshgoebel merged commit a2bfcce into highlightjs:main Mar 2, 2022
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants