-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added CodeBlockComponent #2275
Added CodeBlockComponent #2275
Conversation
Size Change: +1.18 kB (0%) Total Size: 728 kB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though unsure about babel being in dependencies
rather than devDependencies
@@ -65,6 +65,7 @@ | |||
"@typescript-eslint/eslint-plugin-tslint": "^4.1.1", | |||
"ajv": "^6.12.6", | |||
"aws-sdk": "^2.804.0", | |||
"babel-plugin-prismjs": "^2.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt this be in devDependencies
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great spot! Fixed 👍
@@ -24,6 +24,10 @@ module.exports = { | |||
], | |||
'babel-plugin-px-to-rem', | |||
'@babel/plugin-transform-runtime', | |||
["prismjs", { | |||
// This list should match those defined in typescript type Language | |||
"languages": ["typescript", "javascript", "css", "markup", "scala", "elm"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add typescript
to that list. Is that easy to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, first implementation in composer is here: https://github.com/guardian/flexible-content/pull/459. That makes it look like it's using pretiffy which may or may not support TS. To be fair, it might not matter that much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettify is deprecated and I'm not sure if the implementation we have with it on Frontend is working anymore - the code blocks I see there don't seem to have much styling other than a basic pass.
What?
Adds the
CodeBlockElement
to display code snippets inside articles.Support for code snippets has been fairly basic up to now on Frontend, we set a background colour, the font, some spacing and that's about it. In this PR I wanted to update these styles and also add syntax highlighting.
Why?
More readability, better accessibility and so that our articles in the Guardian Digital Blog look nicer
Before
After
Library choice
Given code formatting is a specialist field with mature solutions already available, I decided to use a library. There are three main options available:
highlight.js
Google prettier
and
Prism
Highlight.js
Pros:
Cons:
Google
I list this here because I see references to this in Frontend and it's well known but it is now deprecated.
Prism
Pros:
Cons:
Decision
Use Prism. Bundle size is critical for us and the result even when using the wrong language is still acceptable.