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

[Docs] Do not center math equations and chemical formulas #2081

Merged
merged 4 commits into from
Jun 16, 2020
Merged

Conversation

dkonopka
Copy link
Contributor

@dkonopka dkonopka commented Oct 2, 2019

Suggested merge commit message (convention)

Docs: Do not center math equations and chemical formulas. Closes #2080.


Additional information

http://127.0.0.1:8080/build/docs/ckeditor5/12.4.0/features/math-equations.html

Screenshot 2019-10-02 at 16 48 06

@dkonopka dkonopka requested a review from mlewand October 2, 2019 14:44
@dkonopka dkonopka changed the base branch from master to stable October 2, 2019 14:48
@dkonopka dkonopka changed the base branch from stable to master October 2, 2019 14:49
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

I believe that more reliable solution would be to negate image style customization for any editor's editable.

Easiest hax would be to put :not() in Umberto stylesheet, but it shouldn't know anything about CKE5.

So we need to do it on CKE5 side of things, something like adding: display: initial for .formatted p img and proper negation for other customized CSS. But it should apply to all samples, not just MathType alone.

@dkonopka dkonopka requested a review from mlewand October 3, 2019 06:43
@dkonopka
Copy link
Contributor Author

dkonopka commented Oct 3, 2019

Good catch! Fixed already 🔥

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

The code looks fine and fixes the #2080.

BUT while verifying it I noticed that current block display does a perfect job in covering up bug #1874. So as such… we're better off with displaying it as a block now 🙂

Let's keep this issue open till #1874 is not resolved.

@mlewand
Copy link
Contributor

mlewand commented Oct 3, 2019

@Mgsy Can I ask you to verify whether it doesn't break images in any other samples? What basically happened here is that images in .live-snippet elements are no longer treated as blocks (but instead area inline-block, as in HTML standard).

The reason for adding display block was probably to get a nice full width images in our articles content.

@mlewand mlewand requested a review from Mgsy October 3, 2019 09:18
@mlewand mlewand changed the title [Docs] Do not center math equations and chemical formulas. [Docs] Do not center math equations and chemical formulas Oct 3, 2019
Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

Everything looks fine 👍

@Reinmar
Copy link
Member

Reinmar commented Oct 7, 2019

R- for now due to #2080 (comment)

@mlewand
Copy link
Contributor

mlewand commented Apr 6, 2020

Rebased the branch onto latest master

@mlewand mlewand self-assigned this Apr 6, 2020
@mlewand
Copy link
Contributor

mlewand commented Jun 16, 2020

Rebased once again onto latest master branch. Looks fine and the #1874 is no longer bugging us 👍

@mlewand
Copy link
Contributor

mlewand commented Jun 16, 2020

@ckeditor/qa-team Gents can I ask you to do some post-merge verification to ensure that images in samples are displayed correctly? I gave it some testing, but would like to have QA see it too.

What this PR affects is all the image elements ( <img> ) within .live-snippet (so basically editors in our samples).

Some scenarios that I can see worth testing include:

  • mathtype equation
  • pasting an image
  • pasting embedded content that incorporates an image

@mlewand mlewand self-requested a review June 16, 2020 14:33
@mlewand mlewand merged commit 81a9409 into master Jun 16, 2020
@mlewand mlewand deleted the t/2080 branch June 16, 2020 14:35
@FilipTokarski
Copy link
Member

I checked it and didn't find any new bugs, it seems to work fine 👍

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.

Docs styling breaks Mathtype equation display
5 participants