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 mobile editor font #1797

Open
wants to merge 7 commits into
base: master
from

Conversation

@devonzuegel
Copy link
Contributor

commented Aug 12, 2019

Description

Enables user to select the font they want to use in the Joplin mobile editor.
Partially addresses #337.

What next?

I could add something similar for the rendered note font (i.e. the page before the editor).

@devonzuegel devonzuegel changed the title Custom mobile editor styles Custom mobile editor font Aug 12, 2019

@devonzuegel devonzuegel marked this pull request as ready for review Aug 12, 2019

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2019

I love it. I always despised the default non-monospace font in mobile. I hope Laurent accepts this PR. It's one of the reasons why I don't use the mobile apps very often.

@agmemon10

This comment has been minimized.

Copy link

commented Aug 19, 2019

Could there be a way to use custom fonts in the rendered view using span style tags and importing font faces in style tags at the end of the document? i.e. importing and applying custom fonts using css

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

@agmemon10 this PR has nothing to do with the rendered view.

@agmemon10

This comment has been minimized.

Copy link

commented Aug 19, 2019

@tessus my apologies, I thought that @devonzuegel was referring to using custom font in the rendered view under the "What next?" title in the original post

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

@agmemon10 yes, but that would be a follow up PR. first we need to ascertain, if @laurent22 even accepts this PR. then we have to figure out why it's not working on Android.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Aug 29, 2019

I'm fine with this change, we can merge once it works with Android.

To detect Android from Setting.js you can do it like this:

  • First add a function like "mobilePlatform()" in lib/shim.js which would return "" by default (meaning it would return nothing if we're not on mobile).
  • Then in lib/shim-init-react.js, override this function and make it return Platform.OS (from react-native package)
  • Finally you can call shim.mobilePlatform() from Setting.js to get the info you need.

@devonzuegel devonzuegel force-pushed the devonzuegel:custom-ios-editor-styles branch from 50d5109 to f597353 Sep 8, 2019

@devonzuegel

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Thanks for the pointers @laurent22! That's super helpful. I added the shim, which seems to handle the platform switching appropriately.

Do you know which fonts are allowed for Android? I'd love to support this feature for Android too, since @tessus seems excited about it. But if it's not immediately obvious how to do that, we could go down that rabbit hole in another PR so it's not blocking this one.

UPDATE: It looks like these are the fonts available out-of-the-box: https://github.com/react-native-training/react-native-fonts, though from what @tessus said it sounds like monospace didn't work, so perhaps this list isn't showing what I think it is?

_('This must be *monospace* font or it will not work properly. If the font ' +
'is incorrect or empty, it will default to a generic monospace font.'),
},
editorFont: {

This comment has been minimized.

Copy link
@laurent22

laurent22 Sep 8, 2019

Owner

For consistency, is it possible to reuse the style.editor.fontFamily key? You could conditionally set some properties like isEnum depending on whether we're on mobile or desktop. Then I guess the property would need to be a string, which I think is acceptable? The value would just be the actual font name "Avenir", "Menlo", etc. instead of the constants.

This comment has been minimized.

Copy link
@devonzuegel

devonzuegel Sep 9, 2019

Author Contributor

Oh good catch! I completely missed style.editor.fontFamily above. 🤦‍♀ Thank you, I've just pushed a commit fixing that.

This comment has been minimized.

Copy link
@laurent22

laurent22 Sep 9, 2019

Owner

Looks like the commit isn't here yet? Also note that there's an issue with eslint where it will unstage files when making changes, but not staging them back.

This comment has been minimized.

Copy link
@devonzuegel

devonzuegel Sep 10, 2019

Author Contributor

Whoops here you go: 5c474ca

@laurent22

This comment has been minimized.

Copy link
Owner

commented Sep 8, 2019

@devonzuegel, I've tried to check but the branch throws an exception for me as I think the editor font default value is not right. I've also left a few comments in the code.

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Sep 8, 2019

@laurent22 that's interesting, because I was able to compile and run it w/o issues. except that nothing happened on Android when choosing a font.

@devonzuegel

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Do you have the Menlo font installed locally? My guess is that @laurent22 doesn't have it while both @tessus and I do, which would explain why he's seeing the error in iOS and we're not (which means that it is indeed buggy, since I can't assume folks will have the font, especially on their phones).

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

Yes, I have all fonts installed locally, but that doesn't mean they are available to the iOS or Android emulators, or does it? I would have thought that they had to be installed properly to be referenced.

Hmm, so we either have to ship fonts, which might not be that easy due to licensing and it would increase the app size, or we find fonts that are available on iOS and Android that we can use....
I'm gonna do some research tomorrow on available system fonts on iOS and Android.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Sep 9, 2019

Maybe the issue is that with this change we force a default font but if that default doesn't exist it will fail. Instead, the default should be like now to set no font at all and let the system pick one. So I think the options should be: "Default", followed by OS-specific fonts, "Menlo", etc.

Also I'm thinking, is it really do-able to list specific fonts? On iOS maybe, but on Android there are so many different variants that we can't know in advance if a font will exist or not.

So to keep it simple we could have two sets of options:

iOS

  • Default
  • Menlo
  • Courier New
  • Avenir

Android

  • Default
  • Monospace

For Android, Monospace will just be a CSS string like "Monospace, Courier News", etc. Maybe later we can pick some nice fonts commonly available, but for now just generic monospace would do.

@devonzuegel

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

The two sets of options makes sense. I'll do that. Anything else to finish before this is ready to merge?

Sorry for all the back and forth, during the week I don't have much time. Aiming to get all this resolved and merged this coming weekend!

@laurent22

This comment has been minimized.

Copy link
Owner

commented Sep 10, 2019

I think that's it otherwise. I'll give it a try later to see if I can get a basic monospace font to display in Android.

@devonzuegel

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Ready for final review I think! Thanks for the patience.

@devonzuegel devonzuegel requested review from laurent22 and tessus Sep 15, 2019

@devonzuegel

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Oh also since I haven't been able to get Android to build locally, could someone test that and make sure I haven't broken it?

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Sep 16, 2019

It compiles on Android. However, there's no menu item in the section Appearance, so I can't set a font on Android.

@tessus
Copy link
Collaborator

left a comment

The code works, except that there's no monospace font on Android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.