Adapt syntax highlighting on dark theme #2769

dbelokon · 2022-01-28T19:56:17Z

Issue This PR Addresses

Type of Change

Bugfix: Change which fixes an issue
New Feature: Change which adds functionality
Documentation Update: Change which improves documentation
UI: Change which improves UI

Description

All code blocks that the Telescope back-end sends us are annotated with highlight.js classes, so the front-end has to provide the stylesheet that defines these.

Since the stylesheet is global, we have to link both stylesheets for light and dark theme, and disable either depending on the current theme.

Checklist

Quality: This PR builds and passes our npm test and works locally
Tests: This PR does not includes tests as it does not add new functionality or change it.
Screenshots: This PR includes screenshots or GIFs of the changes made:
Documentation: This PR does not include documentation as it does not add user exposed functionality. However, a couple of comments were provided in the code as well in the commit message to explain certain changes.

gitpod-io · 2022-01-28T19:56:19Z

humphd

I'm thrilled to see us fixing this, thank you!

A couple of things to consider.

humphd · 2022-01-28T20:50:51Z

src/web/src/pages/_app.tsx

+
+    // Enable the stylesheet for the syntax highlighting depending on the theme
+    if (preferredTheme === 'dark') {
+      document.querySelector('#light-stylesheet')?.setAttribute('disabled', 'disabled');


.disabled = true see https://developer.mozilla.org/en-US/docs/Web/API/StyleSheet/disabled

Question: should this happen within a useEffect @DukeManh?

Yeah, it should be useEffect of hooks/use-preferred-theme

humphd · 2022-01-28T20:52:56Z

src/web/src/pages/_app.tsx

  const toggleTheme = () => {
-    setPreferredTheme(preferredTheme === 'dark' ? 'light' : 'dark');
+    if (preferredTheme === 'dark') {
+      document.querySelector('#light-stylesheet')?.removeAttribute('disabled');


We're repeating code here, let's consider moving this enable/disable logic to function(s), and maybe even put it in hooks/use-preferred-theme so it happens when we get/set the theme.

DukeManh

Why haven't we done this earlier. Thank @dbelokon.

One thing, let's use the minified css files to save loading time. https://highlightjs.org/download/

TueeNguyen · 2022-01-29T03:00:16Z

I like these inline format more than the old one

humphd · 2022-01-30T23:56:16Z

src/web/public/styles/github-dark.min.css

@@ -0,0 +1,10 @@
+pre code.hljs{display:block;overflow-x:auto;padding:1em}code.hljs{padding:3px 5px}/*!


Is it possible for us to get these via node_modules/ vs. embedding directly in our code? Then we can get updates.

So, as I understand how next.js deals with static files and CSS style sheets in particular, it might not be possible with the tools we have available...

If your style sheets were in src/web/src/styles, you can @import a style sheet found in node_modules, but next.js modifies the original file so that the @import rule is not there anymore. So, we cannot access the github.css and github-dark.css files from the @import rules like in CSSImportRule. And, even if next.js kept the @import rule, we wouldn't be able to find the original style sheet with an id, because the style element generated by next.js does not have an id.

If you place the style sheets in src/web/public, you cannot @import a style sheet found in node_modules.

So, as of right now, it might not be possible. If someone knows a lot about next.js, please let us know if you can think of a way to make this possible.

In the worst case, we would have to use a dependency that copies certain files in node_modules during next.js compilation time for this specific case.

humphd

I'm surprised there's no way to do this with next.js, but if it can't be done, we can go this route. One question.

src/web/src/pages/_app.tsx

dbelokon · 2022-02-01T03:28:09Z

I'm surprised there's no way to do this with next.js, but if it can't be done, we can go this route.

I am not very experienced with next.js, so I do not know the limits of it and I cannot research more because Google is giving me the same pages I already checked. I could try to ask the next.js developers about something like this, if we do not have someone else that might have an idea

humphd · 2022-02-01T03:29:35Z

I'm surprised there's no way to do this with next.js, but if it can't be done, we can go this route.

I am not very experienced with next.js, so I do not know the limits of it and I cannot research more because Google is giving me the same pages I already checked. I could try to ask the next.js developers about something like this, if we do not have someone else that might have an idea

cc @DukeManh, @Andrewnt219 for thoughts

aserputov · 2022-02-01T03:30:36Z

@dbelokon I can help you. Let's ask people in the chat, otherwise we will think something.

DukeManh · 2022-02-01T03:52:37Z

@dbelokon, I think you misunderstood the import CSS part. In React/Next, CSS files can be imported like a module in JS like so: import 'highlight.js/styles/github.css. What you are talking about is importing a CSS file into another, correct?

@humphd, I don't think importing CSS files from node_modules would work for us because we want to give them ids so we can select and disable them later. I don't know how to give ids to imported CSS files.

humphd · 2022-02-01T03:54:46Z

What if you do dynamic imports and only pull in the one you need, when you need it?

DukeManh · 2022-02-01T04:02:29Z

Would it work for CSS file?
Why don't you give it a try @dbelokon, https://nextjs.org/docs/advanced-features/dynamic-import

dbelokon · 2022-02-01T09:27:39Z

So, it seems that dynamic import might not work either :(

As I understand, it seems next.js treats the import 'path/to/style.css; statements as an exception more than a rule. That means, import() will not work.

The main problem is that the CSS style sheet has to be a global one, because otherwise the elements that are annotated with the class names won't get styled at all. So, if you have two global sheets that apply to the same class names, you need to disable one of them for the other to work. If the styling was bundled in a React component, this would be much easier, but of course, we wouldn't have the problem of the style sheet being global anyway.

humphd

@dbelokon I feel like we're holding up "good" work in an effort to have "perfect," and this is the wrong approach: we should take what you've done, which is awesome, and iterate on it later.

Thanks for entertaining these discussions as we look for a proper way to solve this. It was worth researching.

aserputov · 2022-02-01T16:28:54Z

@dbelokon could you please rebase this, and we can merge it.

dbelokon · 2022-02-01T21:15:09Z

Hahaha thank you @humphd!!

Going to rebase it shortly

dbelokon · 2022-02-01T21:35:42Z

Rebased and squashed without conflicts!

humphd

Why is the light background gray now vs. white?

dbelokon · 2022-02-01T21:59:42Z

Why is the light background gray now vs. white?

I did it because it looked a little bit better, especially with GitHub's syntax highlighting. This is my opinion, though 😅

I can change it back to white background, if you don't mind

dbelokon · 2022-02-01T22:11:18Z

It looks a bit more "separated" from the other content when the code has grey background color vs. white.

Dark theme:

White theme:

humphd · 2022-02-01T22:17:36Z

For a "light" theme, I think white is the way to go. It helps with text contrast with accessibility.

humphd

This is only partially fixed: inline <code> is still gray:

All code blocks that the Telescope back-end sends us are annotated with highlight.js classes, so the front-end has to provide the stylesheet that defines these. Since the stylesheet is global, we have to link both stylesheets for light and dark theme, and disable either depending on the current theme.

dbelokon requested review from humphd, cindyorangis and TueeNguyen January 28, 2022 19:56

dbelokon assigned TueeNguyen and dbelokon Jan 28, 2022

vercel bot deployed to Preview January 28, 2022 19:56 View deployment

dbelokon added this to the 2.6 Release milestone Jan 28, 2022

dbelokon force-pushed the issue-2377 branch from 03c817c to d31ce0b Compare January 28, 2022 20:21

vercel bot deployed to Preview January 28, 2022 20:21 View deployment

humphd requested changes Jan 28, 2022

View reviewed changes

DukeManh requested changes Jan 28, 2022

View reviewed changes

vercel bot deployed to Preview January 30, 2022 23:45 View deployment

dbelokon requested review from humphd and DukeManh January 30, 2022 23:45

dbelokon force-pushed the issue-2377 branch from 2b7b74a to 8234a1b Compare January 30, 2022 23:48

vercel bot deployed to Preview January 30, 2022 23:48 View deployment

humphd requested changes Jan 30, 2022

View reviewed changes

aserputov self-requested a review January 31, 2022 21:20

humphd reviewed Feb 1, 2022

View reviewed changes

src/web/src/pages/_app.tsx Outdated Show resolved Hide resolved

vercel bot deployed to Preview February 1, 2022 03:26 View deployment

sirinoks added the area: design Issues needing design or assets label Feb 1, 2022

sirinoks added the area: front-end label Feb 1, 2022

humphd previously approved these changes Feb 1, 2022

View reviewed changes

aserputov previously approved these changes Feb 1, 2022

View reviewed changes

dbelokon dismissed stale reviews from aserputov and humphd via 9501b60 February 1, 2022 21:35

dbelokon force-pushed the issue-2377 branch from d4407d2 to 9501b60 Compare February 1, 2022 21:35

vercel bot deployed to Preview February 1, 2022 21:35 View deployment

dbelokon requested review from humphd and aserputov February 1, 2022 21:35

humphd reviewed Feb 1, 2022

View reviewed changes

dbelokon force-pushed the issue-2377 branch from 9501b60 to 8dbcb84 Compare February 1, 2022 22:44

vercel bot deployed to Preview February 1, 2022 22:44 View deployment

dbelokon requested a review from humphd February 1, 2022 22:44

aserputov previously approved these changes Feb 1, 2022

View reviewed changes

humphd requested changes Feb 1, 2022

View reviewed changes

dbelokon dismissed aserputov’s stale review via 99f5b4f February 1, 2022 23:32

dbelokon force-pushed the issue-2377 branch from 8dbcb84 to 99f5b4f Compare February 1, 2022 23:32

vercel bot deployed to Preview February 1, 2022 23:32 View deployment

dbelokon requested review from humphd and aserputov February 1, 2022 23:32

humphd approved these changes Feb 2, 2022

View reviewed changes

aserputov approved these changes Feb 2, 2022

View reviewed changes

aserputov merged commit f30ac73 into Seneca-CDOT:master Feb 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt syntax highlighting on dark theme #2769

Adapt syntax highlighting on dark theme #2769

dbelokon commented Jan 28, 2022

gitpod-io bot commented Jan 28, 2022

humphd left a comment

humphd Jan 28, 2022

humphd Jan 28, 2022

DukeManh Jan 28, 2022

humphd Jan 28, 2022

DukeManh left a comment

TueeNguyen commented Jan 29, 2022

humphd Jan 30, 2022

dbelokon Feb 1, 2022 •

edited

Loading

dbelokon Feb 1, 2022

humphd left a comment

dbelokon commented Feb 1, 2022

humphd commented Feb 1, 2022

aserputov commented Feb 1, 2022

DukeManh commented Feb 1, 2022

humphd commented Feb 1, 2022

DukeManh commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd left a comment •

edited

Loading

aserputov commented Feb 1, 2022

dbelokon commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd left a comment

dbelokon commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd commented Feb 1, 2022

humphd left a comment

		@@ -0,0 +1,10 @@
		pre code.hljs{display:block;overflow-x:auto;padding:1em}code.hljs{padding:3px 5px}/*!

Adapt syntax highlighting on dark theme #2769

Adapt syntax highlighting on dark theme #2769

Conversation

dbelokon commented Jan 28, 2022

Issue This PR Addresses

Type of Change

Description

Checklist

gitpod-io bot commented Jan 28, 2022

humphd left a comment

Choose a reason for hiding this comment

humphd Jan 28, 2022

Choose a reason for hiding this comment

humphd Jan 28, 2022

Choose a reason for hiding this comment

DukeManh Jan 28, 2022

Choose a reason for hiding this comment

humphd Jan 28, 2022

Choose a reason for hiding this comment

DukeManh left a comment

Choose a reason for hiding this comment

TueeNguyen commented Jan 29, 2022

humphd Jan 30, 2022

Choose a reason for hiding this comment

dbelokon Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

dbelokon Feb 1, 2022

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

dbelokon commented Feb 1, 2022

humphd commented Feb 1, 2022

aserputov commented Feb 1, 2022

DukeManh commented Feb 1, 2022

humphd commented Feb 1, 2022

DukeManh commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd left a comment • edited Loading

Choose a reason for hiding this comment

aserputov commented Feb 1, 2022

dbelokon commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd left a comment

Choose a reason for hiding this comment

dbelokon commented Feb 1, 2022

dbelokon commented Feb 1, 2022

humphd commented Feb 1, 2022

humphd left a comment

Choose a reason for hiding this comment

dbelokon Feb 1, 2022 •

edited

Loading

humphd left a comment •

edited

Loading