-
-
Notifications
You must be signed in to change notification settings - Fork 834
Conversation
This will need to be a bit smarter as you can specify multiple comma separated fonts. E.g |
@t3chguy I wonder how well this would work in practice? const possible_user_input = "Fira Sans Thin, Commodore 64";
`"${possible_user_input.split(',').map(font => font.trim()).join("\",\"")}"` // "Fira Sans Thin","Commodore 64" Alternatively, we could only do |
The split map join is what I had in mind but please add the double quotes in the map phase to aid readability |
Also worth considering that existing users may have already added double quotes manually |
I wonder how it is now? |
@t3chguy I applied your suggestions. |
Do you think it'd be feasible to write a small test for this improvement? |
Co-authored-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy Sadly I don't have any idea where or how to do that... What would maybe be possible for me is to extract the relevant logic into a function that would be tested. |
So yeah you'd create a file You can probably copy |
I've added a basic template. How do I have to modify |
@r00ster91 look at the other test in the same directory, it does these exact things |
I wonder if something like this would do it. |
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.
Awesome, thanks for your contribution!!
Thanks! |
Right, I just wanted to add those |
Async stuff is always fun, gimme a sec |
Hey @r00ster91 unfortunately one of your tests had a legitimate failure Which makes sense given both sides of the I'm going to update it to get rid of the spurious spaces inside the quotes, if a user put spaces in quotes they probably intended them.. Please review my changes and let me know if you're happy for them to land alongside your changes :) |
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.
Uh-huh, it seems you did some organizational changes too.
I'm surprised to see this PR turn out much more complicated than I initially thought.
It looks fine overall! You might as well add your copyright too.
Haha, I'm about to add some tests into the file as part of another PR #8670 so will do so there
Mostly just to reuse testing utilities around the dispatcher |
Thanks for writing the actual code to make custom fonts less confusing!! |
So for a while I tried to use the Commodore 64 font (the same that VVVVVV uses) in my favorite messenger.
But I always had this problem that I had the font installed and I entered the exact name of the font but it didn't work:
Well it turns out that the problem was the space in the font's name:
Doing this automatically for the user should avoid a lot of confusion.
This change is marked as an internal change (Task), so will not be included in the changelog.