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

Remove unused line height style for mce-content-body #13867

Merged
merged 2 commits into from
Feb 14, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Feb 13, 2019

mce-content-body is no longer applied to editable rich text elements. (I think that changed with #13697). As a result, this style is no longer picked up.

Since this style is unused, there should be no visual difference in removing it. But here are screenshots to show where it used to show up back when it was active:

Before (WordPress 5.0.3)

screen shot 2019-02-13 at 2 55 49 pm

Current (in master, and in this PR)

screen shot 2019-02-13 at 2 54 51 pm


As a sidenote, applying that mce-content-body style to the editor-styles-wrapper was causing some theme style issues like #10067, so it's good to have it removed. (More discussion here) 🙂

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Nice one. This should be a trivial code cleanup since that style isn't used at all anymore.

However, can you go one step further now that you're at it, and also remove this line?

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/rich-text/style.scss#L10

I tried removing it, and the "line-height: inherit" makes sure that the correct line height is inherited from the editor styles wrapper. Which is superior. So might as well kill both while we're at it, right?

Looks good to me:

screenshot 2019-02-14 at 08 03 30

@youknowriad
Copy link
Contributor

@jasmussen This second line also applies to lists, quotes... I wonder if it has an impact there.

@jasmussen
Copy link
Contributor

Really solid thought, Riad. Looks to me like those are fine, and inherit correctly. So really, removing that line is just another feather in the cap for reducing editor style specificity!

screenshot 2019-02-14 at 09 00 03

screenshot 2019-02-14 at 09 00 51

☝️ don't mind the dark style, fixing a bug. And those all inherit the correct lineheights.

@youknowriad
Copy link
Contributor

Thanks for testing @jasmussen 👍

@ellatrix
Copy link
Member

Looks good to me! Just also dropping here that .editor-rich-text__tinymce changed to .editor-rich-text__editable. Please avoid using this selector at all costs!

This line height can be inherited with no negative effects. Removing this allows for less specific rules, making editor styles a bit easier.
@kjellr
Copy link
Contributor Author

kjellr commented Feb 14, 2019

Thanks, everyone!

However, can you go one step further now that you're at it, and also remove this line?

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/rich-text/style.scss#L10

Yep! I removed this in eafac98 and haven't noticed any issues either. I'll go ahead and merge once the tests are finished up. 👍

@kjellr kjellr merged commit a779901 into master Feb 14, 2019
@kjellr kjellr deleted the try/remove-unused-mce-content-body-class branch February 14, 2019 13:58
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Remove unused line height style for mce-content-body

* Remove unnecessary line-height property for .editor-rich-text-editable

This line height can be inherited with no negative effects. Removing this allows for less specific rules, making editor styles a bit easier.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
* Remove unused line height style for mce-content-body

* Remove unnecessary line-height property for .editor-rich-text-editable

This line height can be inherited with no negative effects. Removing this allows for less specific rules, making editor styles a bit easier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants