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

Fix classic block toolbar regression #10443

Merged
merged 3 commits into from
Oct 10, 2018
Merged

Conversation

youknowriad
Copy link
Contributor

Fix a small visual regression in the classic block's toolbar introduced in #10397

@youknowriad youknowriad added this to the 4.0 milestone Oct 9, 2018
@youknowriad youknowriad self-assigned this Oct 9, 2018
@youknowriad youknowriad requested review from gziolo and a team October 9, 2018 16:00
@gziolo gziolo added the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@gziolo gziolo requested a review from jasmussen October 9, 2018 16:02
@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Priority] High Used to indicate top priority items that need quick attention labels Oct 9, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Code wise looks good. I would love to have 👍 from @jasmussen to ensure it looks exactly the same as in 3.9 :)

@jasmussen
Copy link
Contributor

Looking now.

@jasmussen
Copy link
Contributor

Looks good, but it also looks like there's a separate regression.

Master:

screenshot 2018-10-09 at 18 21 56

This branch:

screenshot 2018-10-09 at 18 22 38

3.9:

screenshot 2018-10-09 at 18 24 14

Notice how the paragraphs below, in both cases, has a lineheight of 1.3 as opposed to 1.8 as it should. It looks like a lineheight rule from .mce-content-body is bleeding in:

screenshot 2018-10-09 at 18 24 46

This is supposed to fix it:

screenshot 2018-10-09 at 18 25 17

But obviously that's not working anymore.

Adding to the p itself should fix it:

screenshot 2018-10-09 at 18 25 11

But that's also not a nice fix, especially since we want editor styles to be as close to the stock experience as possible. It would be nice to figure out why that extra rule is suddenly there.

@youknowriad
Copy link
Contributor Author

@jasmussen I'm not having the same results as you in this branch. Could you have tested "master" instead of this branch?

@jasmussen
Copy link
Contributor

Sorry forgot to mention, you have to click the classic block so it initializes, only then the line height issue persists.

This is an issue both in this branch and master and okay to fix separately. I have to help with the kids but can look later.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@gziolo
Copy link
Member

gziolo commented Oct 10, 2018

It looks okey on Desktop viewport.

Mobile experience isn't perfect, but that might be also the case for master:

screen shot 2018-10-10 at 10 54 25

Unrelated issue is in-between lines inserter:

screen shot 2018-10-10 at 10 55 40

You can't even trigger it when hovering on the top of the selected Classic block.

@@ -33,3 +31,7 @@ ul ul,
ol ul {
list-style-type: circle;
}

.mce-content-body {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍

An ideal fix would be to not load /wp-includes/js/tinymce/skins/lightgray/content.inline.min.css at all. I think that should also fix this issue.

But if we can't do that, this seems like the better fix.

@youknowriad youknowriad force-pushed the fix/classic-toolbar-regression branch from a8aa6ad to 979261b Compare October 10, 2018 10:36
@youknowriad youknowriad merged commit 27015c2 into master Oct 10, 2018
@youknowriad youknowriad deleted the fix/classic-toolbar-regression branch October 10, 2018 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants