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

CKEditor uses Arial instead of Open Sans in Basis #4201

Open
quicksketch opened this issue Nov 8, 2019 · 8 comments · May be fixed by backdrop/backdrop#2983
Open

CKEditor uses Arial instead of Open Sans in Basis #4201

quicksketch opened this issue Nov 8, 2019 · 8 comments · May be fixed by backdrop/backdrop#2983

Comments

@quicksketch
Copy link
Member

quicksketch commented Nov 8, 2019

Description of the bug
Basis uses "Open Sans" for it's front-end font, but CKEditor does not load the font, instead it's using the default Arial.

Steps To Reproduce
To reproduce the behavior:

  1. Go to node/add/post
  2. Insert text into the editor. Inspect it with the browser and note it's using Arial.
  3. Save the node.
  4. Inspect the text and note that it's using Open Sans.

This results in ever so slight differences in spacing between CKEditor and the front-end.


PR by @quicksketch - backdrop/backdrop#2983

@quicksketch quicksketch changed the title Discrepancies between CKEditor CSS and Basis CKEditor uses Arial instead of Open Sans in Basis Nov 8, 2019
@quicksketch
Copy link
Member Author

PR filed at backdrop/backdrop#2983.

I'm not 100% sure that adding skin.css is the correct solution. That file contains a lot of other CSS that (probably) won't ever affect CKEditor. However it also includes some key utility classes like .color-inverse, .font-size-xlarge, .font-weight-bold, etc.

@indigoxela
Copy link
Member

That file contains a lot of other CSS that (probably) won't ever affect CKEditor.

It doesn't do any harm.
Another option would be to create an explicit ckeditor stylesheet with only the relevant parts of Basis styles. But that might cause more work than it's worth it.

I can confirm, the patch works fine on a 1.15.x-dev with Basis as the default theme. Content styles are the same as when viewing the node (font, link color...).

@olafgrabienski
Copy link

The PR works for me too!

I'm not 100% sure that adding skin.css is the correct solution.

Another option would be to create an explicit ckeditor stylesheet with only the relevant parts of Basis styles.

I'm undecided about adding the extensive skin.css styles for CKEditor. I guess it's helpful when people use the unmodified Basis theme but it might be confusing / misleading when people use a sub-theme of Basis which usually overrides the original skin.css.

Hm, writing about it, at the moment, I'm in favor of a smaller editor.css with the most important styles which doesn't pretend to be exactly the same as Basis.

PS: Bartik comes with a basic editor.css.

@klonos
Copy link
Member

klonos commented Nov 30, 2019

Hm, writing about it, at the moment, I'm in favor of a smaller editor.css with the most important styles which doesn't pretend to be exactly the same as Basis.

I'm in favor of this too. ...especially after attending this session at DrupalSouth: https://www.youtube.com/watch?v=XMkIjhxt0Jg ...there is no reason to be loading additional css/js when it is not needed.

@olafgrabienski
Copy link

@herbdool As you've added the RTBC tag, what do you think about the approach (adding the whole skin.css vs. a smaller editor.css)?

@quicksketch
Copy link
Member Author

I'm down for making an editor.css, seems like it's likely we'll have other things that need to be pulled into the editor that aren't in the skin.css file in the future. And like @olafgrabienski says, skin.css has other implications when dealing with sub-themes, we could mostly side-step that issue by making a dedicated editor.css.

@jenlampton jenlampton modified the milestones: 1.16.1, 1.16.2 May 20, 2020
@jenlampton jenlampton modified the milestones: 1.16.2, 1.16.3 Jun 17, 2020
@ghost ghost removed this from the 1.16.3 milestone Sep 4, 2020
@ghost
Copy link

ghost commented Sep 4, 2020

I'm removing the milestone since PR needs work and this just keeps getting bumped without any actual progress.

@stpaultim
Copy link
Member

stpaultim commented Sep 29, 2020

This looks like it could be a "good first issue."

The goal would be to create a new PR that adds "ckeditor_stylesheets[] = css/editor.css" to the basis.info file and add the necessary CSS to this new file (css/editor.css) in the Basis theme.

This might also involve identifying and moving other CKEditor specific css from Basis into this stylesheet (which might be difficult for a first time contributor).

We have core/themes/bartik/css/editor.css as a possible reference. If anyone thinks this is not a "Good First Issue," please remove the tag.

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

Successfully merging a pull request may close this issue.

7 participants