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

doc: change color of doctag on night mode #38652

Closed

Conversation

@Ayase-252
Copy link
Member

@Ayase-252 Ayase-252 commented May 12, 2021

Demo:

Screen Shot 2021-05-12 at 19 57 00

Fixes: #38641

@github-actions github-actions bot added the doc label May 12, 2021
aduh95
aduh95 approved these changes May 12, 2021
Copy link
Member

@DerekNonGeneric DerekNonGeneric left a comment

Sweet!

Thanks, you can do .hljs-type if you want to too, but that would also need the light-mode styles too, so probably not in this PR.

/cc @nodejs/documentation as an FYI

@Ayase-252
Copy link
Member Author

@Ayase-252 Ayase-252 commented May 12, 2021

@DerekNonGeneric

I think it is a great idea. I will open other PR to adjust .hljs-type.

@DerekNonGeneric
Copy link
Member

@DerekNonGeneric DerekNonGeneric commented May 12, 2021

@Ayase-252, that sounds like a good idea! Pick a nice color for .hljs-type please.

It should be distinguishable from the JSDoc tag (not the same color). Thanks in advance. :)

@DerekNonGeneric
Copy link
Member

@DerekNonGeneric DerekNonGeneric commented May 12, 2021

/to @Ayase-252 check out what GitHub has done here.

They are like whitish or something in the dark mode, so you may not want to diverge too far from that.

image

That is a gist in darkmode, so check it for yourself.

@Ayase-252
Copy link
Member Author

@Ayase-252 Ayase-252 commented May 12, 2021

@DerekNonGeneric

OK, will check.

Looks like Github uses same color of variable to .hljs-type. Let's do the same to see how it looks in Node.js doc.

@DerekNonGeneric
Copy link
Member

@DerekNonGeneric DerekNonGeneric commented May 12, 2021

@Ayase-252, they use a different syntax highlighter (and it is broken as you can see).

It is probably best to hold off on making the PR if this is challenging, but I will keep you posted if I make any progress.

@Ayase-252
Copy link
Member Author

@Ayase-252 Ayase-252 commented May 12, 2021

@DerekNonGeneric
OK, I have just noticed some tiny "edge case" around multi-line type annotation (see * is somehow white), after making a couple of changes.

Screen Shot 2021-05-12 at 23 21 57

Screen Shot 2021-05-12 at 23 23 51

FYI, vscode seems do the job right.

Screen Shot 2021-05-12 at 23 43 54

I think this PR is free from the minor issue.

lpinca
lpinca approved these changes May 12, 2021
Trott
Trott approved these changes May 13, 2021
Lxxyx
Lxxyx approved these changes May 14, 2021
@jasnell
Copy link
Member

@jasnell jasnell commented May 14, 2021

Landed in 67ecd05

@jasnell jasnell closed this May 14, 2021
jasnell added a commit that referenced this issue May 14, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue May 17, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue May 30, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Jun 5, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Jun 5, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this issue Jun 11, 2021
Fixes: #38641

PR-URL: #38652
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants