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

Desktop: Fixes #10036: Applied font family and font size to RTE #10102

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Desktop: Fixes #10036: Applied font family and font size to RTE #10102

merged 2 commits into from
Mar 15, 2024

Conversation

ab-elhaddad
Copy link
Contributor

Fixes #10036

Behavior:

Font size and font family are applied to RTE

Screenshot

After

Changes:

  • Exported the font family from Setting and added it to NoteBodyEditorProps
  • Added style properties to TinyMCE init()

GSoC Details

@@ -598,6 +598,7 @@ const TinyMCE = (props: NoteBodyEditorProps, ref: any) => {
];

const editors = await (window as any).tinymce.init({
content_style: `* {font-size: ${props.fontSize}px; font-family: "${props.fontFamily}"; line-height: 1.5;}`,
Copy link
Owner

Choose a reason for hiding this comment

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

Why the line height?

Copy link
Contributor Author

@ab-elhaddad ab-elhaddad Mar 11, 2024

Choose a reason for hiding this comment

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

For some reason, the lines overlay on each other when the font size increases, So the line-height is going to prevent the overlay.
overlay

Copy link
Owner

Choose a reason for hiding this comment

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

Ok but a fixed size of 1.5 means the lines are going to be too tall for small fonts and still too small for large fonts. How is that solved in the Markdown editor or viewer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.5 is not a fixed size. In CSS, when using a number without a unit, it changes depending on another attribute (in our case, font-size). 1.5 in our case means that the line-height would be 1.5x font-size.

Choose a reason for hiding this comment

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

The original line-height for body in the Rich Text Editor is actually 1.6em:

image

so I think you need to set it to 1.6em here also to keep the same look.

Copy link
Owner

@laurent22 laurent22 Mar 11, 2024

Choose a reason for hiding this comment

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

If it's already set in the body, why does it need to be set in content_style too? Or is this variable replacing (instead of overriding) the style? I'm wondering if we're losing certain important styles by doing this.

Also what if the font family is not set in the settings, what default does it use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 1.6em is actually not a static value; it comes from the themeStyle. So, I used the same value in the TinyMCE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurent22
I will investigate whether it is replaced or overridden.
If the font family is not set in the settings, it takes the default font family, the same as what the code mirror does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurent22

  • The 1.6em in the body is not applied to the text in the editor because the TinyMCE editor creates an iframe to isolate the editor's content from the rest of the application. This iframe contains its own HTML document with its own style, so the line-height of the main body doesn't affect the elements in the iframe.

CSS

An alternate solution would be to give the body of the iframe (#tinymce) a line-height: inherit;

inherit.mp4

Copy link
Owner

Choose a reason for hiding this comment

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

That's great, thank you for looking into this. Indeed if it's an iframe it's not going to get parent style.

@@ -464,6 +464,7 @@ function NoteEditor(props: NoteEditorProps) {
noteToolbarButtonInfos: props.toolbarButtonInfos,
plugins: props.plugins,
fontSize: Setting.value('style.editor.fontSize'),
fontFamily: Setting.value('style.editor.fontFamily'),
Copy link
Owner

Choose a reason for hiding this comment

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

You're passing the fontFamily as a property, but you could just get it from the theme (just like for theme.lineHeight), so could you change this please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to do so, I will have to change the fontFamily property in the globalStyle object in theme.ts, because it's given a static value ('Roboto').
So, should I change it and use theme.fontFamily or stay withprops.fontFamily?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh you're right, it should indeed come from the settings since it's dynamic. All good then!

@laurent22 laurent22 merged commit 85d98f5 into laurent22:dev Mar 15, 2024
10 checks passed
@ab-elhaddad ab-elhaddad deleted the apply-font-size-family branch March 17, 2024 15:55
laurent22 added a commit that referenced this pull request Mar 20, 2024
…TE (#10102)"

This reverts commit 85d98f5.

Reason: Introducing too many issues, some of them know, some of them unknown.
@laurent22
Copy link
Owner

Reverted in eb06ac6

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

Successfully merging this pull request may close these issues.

Editor font size and font family settings are not applied to RTE
4 participants