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

Block Editor: Remove legacy "editor-" class name compatibility #19050

Merged
merged 3 commits into from
Dec 11, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Dec 10, 2019

Related: #18499
Effectively reverts #14420

This pull request seeks to remove the editor--prefixed legacy className compatibility. These were added in #14420 as a result of a migration of components from the @wordpress/editor to @wordpress/block-editor package. With the changed in #14420, class names were changed to adhere to the CSS naming conventions, while retaining the previously-existing editor- prefixed names. As of #18499, a backwards-compatibility statement has been added, clarifying that these classes are not treated as a public interface intended for backwards-compatibility. As such, they are proposed to be removed here.

This brings with it a few benefits:

  • There is no longer any ambiguity on what is the intended canonical class name for these components. You can see in many of the updated files that there was inconsistency in how we referenced these classes, even within first-party code.
  • Clarity in how new classes should be assigned to components, where previously it might have been expected that the editor- prefix be included for the sake of consistency, even when it was not strictly necessary for backwards-compatibility.
  • Reduced bundle size. There appears to be ~4kb reclaimed from these changes in the block-editor bundle (331kb to 327kb minified, before gzip).

Implementation Notes:

My process was as follows:

To find the legacy classes, I performed a regular expression search on the files of the packages/block-editor folder, using the pattern (?<!block-)editor-. For each of these results, I manually verified and removed each compatibility class.

Once all the changes had been made, I compiled a complete set of folders where files had been modified, to use as the "Base" of our BEM naming convention to perform a search on the entire repository for lingering references to the legacy class names:

(?<!block-)editor-(compare|drop-zone|block-icon|block-inspector|block-contextual-toolbar|block-list|block-mover|block-navigation|block-settings-menu|block-styles|block-switcher|block-toolbar|block-types-list|color-palette|contrast-checker|default-block-appender|inner-blocks|inserter-with-shortcuts|inserter|link-control|media-placeholder|media-replace-flow|multi-selection-inspector|panel-color-settings|plain-text|rich-text|skip-to-selected-block|url-input|url-popover|warning|writing-flow)

This yielded many results where one of the following holds true:

  • We were erroneously not using the correct, canonical class name (example)
  • The code is old and unused (example)
  • Class names were assigned to wrongly reuse styles from the block-editor package (example). Note: I did not update these issues here.

Testing Instructions:

Repeat testing instructions from #14420.

@aduth aduth added [Type] Performance Related to performance efforts Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Code Quality Issues or PRs that relate to code quality Needs Dev Note Requires a developer note for a major WordPress release cycle labels Dec 10, 2019
@ellatrix
Copy link
Member

Was there a dev note when the new class was introduced?

@aduth
Copy link
Member Author

aduth commented Dec 10, 2019

Was there a dev note when the new class was introduced?

When they were introduced, it was the expectation that existing code would continue to work without any revisions, so a devnote wasn't deemed as being necessary. Since there could be revisions necessary as a result of this pull request, I've flagged it "Needs Dev Note" based on guidance from the backward compatibility document.

@youknowriad
Copy link
Contributor

youknowriad commented Dec 11, 2019

Actually we wrote a global dev note about it even when we introduced them (editor to block-editor). A new one is needed as well.

@aduth
Copy link
Member Author

aduth commented Dec 11, 2019

Actually we wrote a global dev note about it even when we introduced them (editor to block-editor).

Aha! I must have forgotten about this, but you're correct:

https://make.wordpress.org/core/2019/04/09/the-block-editor-javascript-module-in-5-2/

The components that moved from the editor module to the block-editor module are internally using CSS class names that follow the package they’re declared in. For example, the editor-inserter__toggle class name is now renamed block-editor-inserter__toggle. The old class names have been retained to minimize any backward compatibility concern, but the CSS styles are now targeting the new class names. Ideally, you should rely on components in your own code instead of the internal class names used in the WordPress admin. But if you do use those classes, make sure to rename them accordingly.

@aduth aduth merged commit ae05adb into master Dec 11, 2019
@aduth aduth deleted the remove/editor-classname-compat branch December 11, 2019 15:14
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
@aduth aduth mentioned this pull request Jan 7, 2020
5 tasks
@aduth
Copy link
Member Author

aduth commented Jan 10, 2020

I've noticed that a lot of the default themes still reference many of the old class names.

Example: https://core.trac.wordpress.org/browser/trunk/src/wp-content/themes/twentytwenty/assets/css/editor-style-block.css?rev=46722#L233

@mapk Do you have any suggestions on how we'd want to address this? I could open a Trac ticket for the themes to be updated. I'm not sure how that works with respect to if and how theme updates apply for older versions of WordPress (i.e. since these className changes were introduced in WordPress 5.2, what impact this has on if and how the theme is updated for WordPress 5.0 and 5.1).

I think this might explain some of the visual oddities I've been observing when using recent versions of Gutenberg with default themes.

At worst, it's something where we could revert the changes (and in #19489) until we figure out a better solution. But this puts us at odds with our own guidelines and kicks the can further down the road.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants