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

Improve bullet figure #36

Merged
merged 1 commit into from
Mar 24, 2021
Merged

Improve bullet figure #36

merged 1 commit into from
Mar 24, 2021

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Mar 23, 2021

The bullet figure U-25cf displays correctly on Windows, is there a reason to use * instead?

Ubuntu 20.10 Gnome terminal:

unix_1

Windows 10 cmd.exe (CP850):

windows_1

@sindresorhus
Copy link
Owner

It definitely did not display correctly when I made this package. The problem with cmd is that it might support such characters if you have the correct font installed, from what I can remember. I assume you're not testing on a fresh WIndows 10 installation, so your result may or may not be representable.

Some more info: https://stackoverflow.com/questions/388490/how-to-use-unicode-characters-in-windows-command-line

Another possibility, this was fixed in Node.js or rather libuv, by switching from File I/O to Console I/O, or something.

@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 24, 2021

I am using a fresh Windows 10 installation in VirtualBox, specifically 10.0.17763. I am keeping it fresh and not installing anything (including fonts, but also any other software), for testing purpose.

Note: I ran cmd /k chcp to confirm the code page was 850.

It seems like the character displays correctly as far as at least Node 0.10, so I would assume this might not be related to Node.js nor libuv:

bullet

I am wondering whether the difference you experienced might due to an improvement of the default cmd.exe font in a Windows 10 patch release? If this is the case, would it be fair to assume Windows 10 users would have this patch release, considering Windows updates automatically (or gives constant reminders about it)?

@sindresorhus
Copy link
Owner

I am wondering whether the difference you experienced might due to an improvement of the default cmd.exe font in a Windows 10 patch release? If this is the case, would it be fair to assume Windows 10 users would have this patch release, considering Windows updates automatically (or gives constant reminders about it)?

That is the most likely scenario, yes.

index.js Outdated
@@ -79,7 +79,7 @@ const fallback = {
circleCross: '(×)',
circlePipe: '(│)',
circleQuestionMark: '(?)',
bullet: '*',
bullet: '',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do like

bullet: '●',
? It makes it easier to read the code as you can easily see that they're the same.

Copy link
Collaborator Author

@ehmicky ehmicky Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean using the following?

	bullet: main.bullet,

If so, I can do this 👍

I can also update it in the other PRs.

Also, I can submit an additional PR to use this pattern for all existing characters that are common in main and windows. At the moment, there are still a few which are identical but do not use this pattern (for example line).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I meant. I must have copied the wrong URL.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I can submit an additional PR to use this pattern for all existing characters that are common in main and windows

Sure. I think one PR for all those changes is best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

It turns out line was the only figure not using this pattern. I submitted a PR at #44
I also fixed all the current PRs not using this pattern 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants