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

Improve syntax colors #1093

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

atomiks
Copy link

@atomiks atomiks commented Jul 26, 2018

Hey, so I think the syntax colors of the code on the docs could be improved a bit. There are some problems with visual distinctions currently that makes the code a bit harder to read than it could be.

Numbered issues

  1. Operators and variables/plain text are not distinct enough
  2. Numbers and functions are too visually similar
  3. Keywords and props should be distinct
  4. The line highlight should be lighter than the background in a dark theme (inverse of white, which would have a darker color as the highlight). The current dark highlight looks more like a well than a highlight.
  5. Expressions in props are colored red when they should be white (consistent with the child node below: {props.value})

Other

  • Increase line-height slightly to 1.6 instead of 20px
  • General aesthetic changes that makes the code more vibrant and attractive (imo)
  • I also feel like there could be more granularity in the actual parser to allow for more color distinctions, but it's mostly okay I think.

@facebook-github-bot
Copy link
Collaborator

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@reactjs-bot
Copy link

Deploy preview for reactjs ready!

Built with commit d4b2a7e

https://deploy-preview-1093--reactjs.netlify.com

@facebook-github-bot
Copy link
Collaborator

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Member

gaearon commented Aug 13, 2018

What do you think about trying Night Owl by @sdras?
https://marketplace.visualstudio.com/items?itemName=sdras.night-owl

Maybe we could use it.

@AlmeroSteyn
Copy link
Collaborator

A great PR to score even better on accessibility for the docs as a whole. So my two cents:

  1. Colour contrast of the text to background should be sufficient. There is a tool for that (https://developer.paciellogroup.com/resources/contrastanalyser/). If it passes on level AA then most user should be able to easily read it.
  2. The font itself. A good test is that it shows clear difference between capital i and smaller case L, between zero and O and between rn and m.

Night Owl scores pretty good in this from what I can see. Only the comments could be missed by many due to the very low contrast so I'd bump that up and the ligatures could cause trouble for readers with dyslexia.

@atomiks
Copy link
Author

atomiks commented Aug 14, 2018

Night Owl looks beautiful, the problem is that the highlighter's grammar is not granular enough (e.g. can't differentiate between tags and components):

@gaearon
Copy link
Member

gaearon commented Aug 14, 2018

(e.g. can't differentiate between tags and components):

Is that really important though?

@atomiks
Copy link
Author

atomiks commented Aug 14, 2018

You miss out on that nice orangered component color that her theme has 😜

@gaearon
Copy link
Member

gaearon commented Aug 14, 2018

Maybe we can improve the parser later :-) Doesn't have to block from rollout if otherwise everybody likes how the theme looks on the website, and it's more accessible.

@atomiks
Copy link
Author

atomiks commented Aug 14, 2018

Alright, would you like me to change the PR to use Night Owl colors? (And also change the code background color -- which accidentally wasn't committed here)

Where is the parser located? The granularity is mostly fine except for that single problem which makes a big difference to the overall vibe imo. Is it possible to make a quick change to the parser to add a component class name (detecting if the first letter is uppercase?). Then we could rollout the change including the juicy tomato color 🍅

@gaearon
Copy link
Member

gaearon commented Aug 14, 2018

Alright, would you like me to change the PR to use Night Owl colors? (And also change the code background color -- which accidentally wasn't committed here)

Let's do it in a separate PR so we can decide?

Where is the parser located? The granularity is mostly fine except for that single problem which makes a big difference to the overall vibe imo. Is it possible to make a quick change to the parser to add a component class name (detecting if the first letter is uppercase?). Then we could rollout the change including the juicy tomato color 🍅

I'm not certain. We seem to use Prism for highlighting. So it probably comes from gatsby-remark-prismjs plugin. I see prismjs here. Maybe we can contribute to this package. Or fork it and point lockfile to our fork using Yarn resolutions. Either would be fine to me.

@atomiks
Copy link
Author

atomiks commented Aug 14, 2018

Looks like a PR was made very recently: PrismJS/prism#1519

In this case a component alias instead of class-name would be better

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

Successfully merging this pull request may close these issues.

5 participants