-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix(nodejs): use e718 as the default of symbol in node configuration #3533
Conversation
Tested on the following on Arch:
Works on all of them except for Hyper and Xfce Terminal, and the rendering was broken on those before this change anyways. |
Tested on MacOS under the following terminals:
Works on all of them. |
I would appreciate it if someone could test this on Windows, since my current setups are based on virtualized systems which do not support OpenGL. At the least, I'd like to see Windows Terminal, one of the OpenGL terminals (Alacritty, WezTerm, Kitty), and, if possible, the standard conhost. |
Windows Terminal, Alacritty and WezTerm seem fine. conhost didn't list my nerd fonts as a font option and was thus broken. |
Okay, in that case, all we need to do is make the appropriate edit in |
…uration in docs/presets
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.
Unlucky for us, the new symbol is part of the NerdFonts codepoint leakage reported in 2019. I don't have a feel for how close they are to fixing that issue, and I don't think it should be a blocker, but we should make a mental note that this is a potential problem for anyone who uses a language that uses CJK codepoints.
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.
Some confusion over whether my browser or Nerd Fonts is broken at the moment, but either way I don't think it's a big enough reason to hold this PR back.
If it turns out that this is, in fact, a codepoint leakage, we'll probably need to add an FAQ explaining what's going on.
Thank you for your contribution @TrickyPi and thank you for the indepth testing @chipbuster and @davidkna. |
…tarship#3533) * fix: use e718 as the default of symbol in node configuration * wip: change nodejs symbol in docs/config & add a nodejs symbol configuration in docs/presets * wip: update CONTRIBUTING.md
Description
Use e718 as the default of symbol in node configuration
Motivation and Context
Closes #2546
Screenshots (if appropriate):
How Has This Been Tested?
Visually checked on Iterm2, hyper and mac terminal.
Checklist: