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

✨ Custom Fonts #96

Merged
merged 5 commits into from
Dec 2, 2024
Merged

✨ Custom Fonts #96

merged 5 commits into from
Dec 2, 2024

Conversation

STSMHQ
Copy link
Contributor

@STSMHQ STSMHQ commented Nov 16, 2024

Hi, @nienow, as I said on #95, I would like to start contributing more to your repositories as I love Standard Notes product. I'm more a CI guy than a software developer, especially when it comes to frontend stuff, but I've hope that, with your help and guidance, I can make some good progresses.

This PR is an attempt to bring custom fonts to your Quill plugin, based on the code that you have already developed. Please take a look when you've the time and let me know what changes I must make to align it more closely with your standards.

@STSMHQ STSMHQ force-pushed the dev branch 4 times, most recently from e39d652 to e64d325 Compare November 16, 2024 15:37
@nienow
Copy link
Owner

nienow commented Nov 17, 2024

Thanks for another contribution. It looks good, but I see one issue, where the font selection doesn't change automatically back to Sans Serif when you select plain text.

  1. Write some text with the default Sans Serif font.
  2. Change some of the text to Monospace
  3. Select some of the original text that you didn't change, and the font selection still says Monospace.

If you try it on the official site, it seems to work correctly:
https://quilljs.com/playground/snow#codeN4Ig9AlgdgJgpgDwHQAsAuBbANiAXCAHhggDcACCGAXgB0Q5i0B7AJzoD4CxiT2ap+BAM4BjFhAAOaMkJYja4aPGQArIRy6jxU9iAA0i2IiRq8IEUyhDpARwCuELFjJUyUOAHcyARQdOAFADkAMQMEMwsgXpkwPxkZBhMMHZYcEK4MXHxZMxMWABGAIYsGQDaWdlkpcBkKHCF8CVVAIzRAEzRAGaFWEJwALpkAL79ehXZpYH5eTBRZIHhPRAic4F2RixY0HCBo+PxkxAYhQDmO9GBFvAAtPlYTCIA1rtjUJUHNYGdlmiBZYMjV6VPZvYZAsgSLCFERwFAzOBNQIAYSYGAkTD6ZEKbzgEmWSAJUSyaDqGDgGUCQigTA8czAYDIrHm+Ts+TuO34QwAlABuEBDIA

This isn't a big deal, so if we can't figure it out, that's ok.

Also, I see you are using Yarn, which is ok, but the "pnpm-lock.yaml" file should be updated before merging. If you don't want to use pnpm, I can update the file before merging.

I'm not too picky on coding, so its usually fine as long as it works, and doesn't mess up anyone's existing notes.

@STSMHQ STSMHQ force-pushed the dev branch 7 times, most recently from f85d6ab to 227c277 Compare November 17, 2024 12:43
@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 17, 2024

Hi, @nienow, I believe the problem was that I forgot to set a default format for Quill at the beginning of code. I’ve now included a default format with the serif font, and it seems like everything is working fine now. I think It's not perfect yet, as I needed to remove your test data to force the Serif font at the beginning, but as from that initial point, from my understanding, data will be loaded using ops, so I believe it's a valid solution.

The only serious problem seems to be that I needed to roll-back Quill to 1.3.7, as with 2.0.2 version, when adding a new line, the selected font is not persisted and the editor assumes a default unknown font, but as far as I know, this problem is also happening in the playground you just shared, so it may be a bug from the framework itself. I would like to request you to please take a look, once you've the needed time, of course, as you're way more experienced than me.

Regarding the package manager, given your substantial experience in web development, as demonstrated by your repositories, could you kindly provide a brief overview of why you opt for pnpm over yarn v2 (or npm) and what makes it your preferred option?

@nienow
Copy link
Owner

nienow commented Nov 17, 2024

For the package manager. I was using yarn for a while, and I still like it the concept of it, but I had issues with certain projects or libraries using the "pnp" mode. And changing to the "node-modules" mode seemed like it wasn't any better than using "npm". The reason I like "pnpm" is that I have a lot of projects on my computer, and many of them use the same dependencies. pnpm only downloads a dependency once, caches it, and then uses a symbolic link in each project, so it is faster and uses less file space. It also uses the same node_modules format as npm so there is no compatibility issues.

I will look at the code changes later.

@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 17, 2024

That's dope! I knew that Yarn V2 offers the pnp mode for nodeLinker, but I tend to favor the enableGlobalCache option to have everything readily available in the project, particularly since I'm working from a container. Still, I'll explore pnpm further and begin to use it more seriously soon enough, as it seems I can use it directly from corepack the same way as Yarn.

I will look at the code changes later.

Take your time, @nienow, and thank you in advance.

@nienow
Copy link
Owner

nienow commented Nov 18, 2024

I found the default Font whitelist for quill in their code, and it is [false, 'serif', 'monospace'].

So I tried putting false as the default value in a simple example, and it seems like it solves the issue, without any of the other extra code. I was running a simple example here: https://stackblitz.com/edit/stackblitz-starters-zgrzbr?file=index.html

I wish the documentation was better, because this is not easy to figure out!

One more thing, I have the default font set to be whatever the standard notes theme is set to: font-family: var(--sn-stylekit-editor-font-family);. So maybe we make the first option say "Default" instead of "Sans Serif"?

@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 18, 2024

Hi, @nienow! First of all, thank you for introducing me to the Stackblitz. I was unaware of its existence, and it seems to be a great tool to play with.

Unfortunately, I couldn't achieve the same behavior with the demo.html provided in the project. Adding the false variable to the fonts array results in no font name being displayed in the picker - example - (which could be due to additional changes I've done in the CSS file of course). Please check it yourself when you've the needed time.

I've committed a patch that adds an entry to the fonts array to denote the default font - example. Feel free to change anything that I've done in this PR. I'm here to learn from you. Regarding the CSS options that are inherited, do you have any clue of why the theme is not being correctly inherited on Android? On desktop, it's working great.

@nienow
Copy link
Owner

nienow commented Nov 20, 2024

I pushed some changes. I think the main change needed was the removing the [data-value=default] from the css selector. Then we can use "false" as the first font value, and seems to work for me. Let me know if I'm missing something.

@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 20, 2024

Hi, @nienow, I tested it locally, and everything seems to be working great! However, just to let you know, I did notice that when we choose a custom font and press enter to add a new line, the font reverts to Default. If this is the behavior you intended, I’m totally fine with it!

@nienow
Copy link
Owner

nienow commented Nov 21, 2024

choose a custom font and press enter to add a new line, the font reverts to Default

That is annoying. And actually in v1, it looks like it still maintains the Font, it just says "Default" until you start typing. But it v2, it was actually changed to reset all format settings on Enter press: slab/quill#3428. People were complaining about this change.

It looks like if we want to stick with v2, we can override the enter key handler to make it act like v1. Not sure what else we can do.

@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 23, 2024

Another option regarding the enter key handler could be to check the HTML element value whenever Quill doesn't have a font value, like I did here. I'm pretty sure it's not the best approach in terms of performance, though. It's up to you, @nienow. We can go back to version 1.3.7 until this issue is handled in v2.

@nienow
Copy link
Owner

nienow commented Nov 27, 2024

I added the code in the enter key handle to make it keep the selected font after pressing enter, and re-upgraded to v2.

I think this is the best way, even though its not perfect.

Can you test one more time?

@STSMHQ
Copy link
Contributor Author

STSMHQ commented Nov 29, 2024

Hi, @nienow, first of all, sorry for the delay. I'm currently on vacation, so I can't test it out locally. But I totally trust your expertize! Feel free to merge it, and I'll start using the new version on my phone right away.

@nienow nienow merged commit 7a5a2ae into nienow:main Dec 2, 2024
@STSMHQ STSMHQ deleted the dev branch December 4, 2024 18:03
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