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: CJS/ESM switch label is truncated #41878

Closed
mscdex opened this issue Feb 6, 2022 · 8 comments · Fixed by #41885
Closed

doc: CJS/ESM switch label is truncated #41878

mscdex opened this issue Feb 6, 2022 · 8 comments · Fixed by #41885
Labels
doc Issues and PRs related to the documentations.

Comments

@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2022

Affected URL(s)

any

Description of the problem

The CJS/ESM switch label seems to be truncated. Specifically the lower part of the J in CJS is cut off, so it looks like CIS. It seems to be an issue with the SVG itself as it looks the same way when i open the SVG by itself and when I look at it in both Firefox and Chrome (on Linux).

cjs-esm-switch

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Feb 6, 2022
@Trott
Copy link
Member

Trott commented Feb 7, 2022

@mscdex I'm not seeing this problem.

image

Is there anything unusual about your setup that might be a factor? Normally, I'd ask if it was browser-specific but you've already indicated that you're seeing this in Firefox and Chrome.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2022

Is there anything unusual about your setup that might be a factor?

No, just plain old Chrome and Firefox.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2022

I'm guessing it's related to the font being selected since the SVG is using "sans-serif" for the font (which in Chrome translates to "DejaVu Sans"). If I change it to something like "monospace" instead, then nothing gets cut off (although it looks a bit different obviously due to the change in font, which translates to "DejaVu Sans Mono").

@benjamingr
Copy link
Member

Do you also see it if you open the SVG directly? https://nodejs.org/api/assets/js-flavor-cjs.svg

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2022

@benjamingr the problem still exists when opening it directly, which is what I meant earlier. The text labels and the font family references are in the SVG itself.

I think the best solution that will work everywhere going forward is to convert the text to SVG paths, so that the letters are drawn the same, no matter what system fonts the browser is using.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 7, 2022

I took a quick stab at converting the letters to paths using the FreeSans font as the basis for the shapes:

js-flavor-cjs-3
js-flavor-esm-3

I don't know what font the author of the original SVG used so it may not look 100% the same as that, but this looked reasonable to me. The SVG size is doubled, but that's not too bad I think and I've already run them through SVGO. If we serve up gzipped versions of the SVGs, then they should be about the same size as the current SVGs (~700 bytes).

Thoughts?

@benjamingr
Copy link
Member

Thoughts?

LGTM

@aduh95
Copy link
Contributor

aduh95 commented Feb 7, 2022

I don't know what font the author of the original SVG used

That was me, and I didn't use any particular font, so FreeSans SGTM – to be honest I'm even surprised it took so long so someone to complain. Thanks for putting the effort.

mscdex added a commit to mscdex/io.js that referenced this issue Feb 8, 2022
mscdex added a commit to mscdex/io.js that referenced this issue Feb 8, 2022
nodejs-github-bot pushed a commit that referenced this issue Feb 10, 2022
Fixes: #41878

PR-URL: #41885
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41878

PR-URL: nodejs#41885
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
bengl pushed a commit to bengl/node that referenced this issue Feb 21, 2022
Fixes: nodejs#41878

PR-URL: nodejs#41885
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit to danielleadams/node that referenced this issue Apr 21, 2022
Fixes: nodejs#41878

PR-URL: nodejs#41885
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Derek Lewis <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants