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

Add some emoji spinners #46

Merged
merged 15 commits into from
Mar 18, 2021
Merged

Add some emoji spinners #46

merged 15 commits into from
Mar 18, 2021

Conversation

nnmrts
Copy link
Contributor

@nnmrts nnmrts commented Jun 24, 2020

I played around with some emoji and emoji sequences, and this is the result.

I played around with some emoji and emoji sequences, and this is the result.
spinners.json Outdated Show resolved Hide resolved
spinners.json Outdated Show resolved Hide resolved
spinners.json Outdated Show resolved Hide resolved
spinners.json Outdated Show resolved Hide resolved
spinners.json Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

These are all very nice! Thanks for making them. 🙌

spinners.json Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

If it was not clear, this is waiting for #46 (comment)

@sindresorhus
Copy link
Owner

@nnmrts Bump :)

@nnmrts
Copy link
Contributor Author

nnmrts commented Oct 2, 2020

@sindresorhus, ye, sorry, I'll look into it again tonight.

Base automatically changed from master to main January 23, 2021 08:45
@sindresorhus
Copy link
Owner

@nnmrts Bump :)

@nnmrts
Copy link
Contributor Author

nnmrts commented Mar 13, 2021

tl;dr everything should be fixed now, some things were removed because they are dumb, the grenade spinner was updated as well because its first two frames were shorter and i added spaces at the end of all the emoji frames because the existing emoji spinners also have one space at the end; these spinners all have constant widths in a terminal, but not always in browsers; if that's an issue i'll remove those too

EDIT: however, it seems like they are fine in a browser too: https://jsfiddle.net/94o8sjq3/


So, revisiting this, I don't really know how to continue in a sound way. Excuse my language but it seems like string and character width handling in the context of emoji is a complete clusterfuck. Different browsers, different terminals, different programming languages and different libraries all do it differently. I can't even visually compare these spinners in my IDE because VSCode also thinks for some reason it's a good idea that emoji have a slightly larger width than any other characters in an environment that specifically uses fixed-width characters, Terminus and the macOS Terminal think that too, Hyper on the other hand doesn't. All this probably changes with different fonts as well. This is literally untestable using normal spaces and emoji.

Anyways, considering this, and that this projects targets are mainly terminals, I added a constant width test using string-length that seems to be solid at checking the visual constant width of strings for all terminals. I also replaced "normal" spaces with ideographic spaces where it's necessary. I copy-pasted them literally, because then you can actually visually see and compare the real lengths of these strings. This may lead to confusion though, I can also change them to \u+3000 to make it clearer these aren't normal spaces, if you want me to.

Of course this wasn't enough, for some reason ☝️, ✌️ and 🖐️ don't have the usual emoji width in the terminals I tested, they are about a normal space width shorter. They have the usual width in VSCode and browsers though. They also aren't ZWJ sequences or keycap sequences. I now removed those emoji from that spinner. This makes no sense at all though. At least with emoji like 1️⃣, it makes some sense that they behave different, because they are keycap sequences, but it still doesn't make sense that their emoji representations have completely different widths.

Oh shoot, did I mention that the math spinner is also completely broken and unfixable? I didn't? Well, the math spinner is also completely broken and unfixable. One could theoretically implement a parity test for keycap sequences (if there's an even number of these fuckup emoji in a string, they cancel each other out and the resulting string width should be predictable again), but I just called it a day at this point and completely removed this beautiful math spinner. Sad.

I never want to do this again, I regret this whole PR and I wish all the implementers of all of these text editors and displayers all the luck in the world figuring this out.

Thanks for coming to my ted talk.

@nnmrts nnmrts requested a review from sindresorhus March 13, 2021 00:51
test.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Oh wow. Haha. I'm sorry you had to suffer through that...

@sindresorhus
Copy link
Owner

I can also change them to \u+3000 to make it clearer these aren't normal spaces, if you want me to.

👍 Yes

@sindresorhus
Copy link
Owner

Can you fix the merge conflict?

@nnmrts
Copy link
Contributor Author

nnmrts commented Mar 17, 2021

Can you fix the merge conflict?

I hope I did this right. I don't know when the last time was I resolved merge conflicts through GitHub.

But the tests seem to pass, everything seems fine, I think this is it. Yay!

@nnmrts nnmrts mentioned this pull request Mar 17, 2021
@sindresorhus sindresorhus changed the title Add some emoji love Add some emoji spinners Mar 18, 2021
@sindresorhus sindresorhus merged commit 1e1d532 into sindresorhus:main Mar 18, 2021
@sindresorhus
Copy link
Owner

Woot! 👍🏻

@mainrs
Copy link

mainrs commented Mar 18, 2021

The math spinner was really nice :( Maybe I'll go through the suffering to get it back into the package.

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.

3 participants