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

Site Editor: Add Missing Styles #26958

Closed
wants to merge 2 commits into from

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Nov 13, 2020

Description

  • Add the .block-editor class to the Site Editor.
  • Load experimental global styles in the Site Editor.

The Site Editor is initialized in a very different way compared to the Post Editor, and there are plenty of inconsistencies to be found.

I stumbled upon this issue when trying the HTML block in the Site Editor: its textarea didn't have any styles applied.
I then noticed that some styles depend on the .block-editor class, which is created by Core WordPress when rendering edit-post. edit-site is not initialized in the same way, and didn't have that class, making a few components and blocks (e.g. PlainText) not working as intended.

After fixing the plain text view of the HTML block, I also noticed that its preview was broken as well.
As it turned out, we don't add the experimental global styles to the block-editor settings in edit-site.
I'm not sure why, though, and I can't tell if I'm causing any regressions here.
Pinging @nosolosw as author of the original PR for some pointers: #24250

How has this been tested?

  • Open the Site Editor.
  • Add the HTML block, and make sure it's styled appropriately.
  • Insert some HTML, and click on "Preview" in the block toolbar.
  • Make sure the preview is styled appropriately.

Screenshots

Before After
Screenshot 2020-11-13 at 12 34 40 Screenshot 2020-11-13 at 12 32 44
Screenshot 2020-11-13 at 12 34 46 Screenshot 2020-11-13 at 12 32 53

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

Size Change: 0 B

Total Size: 1.19 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.77 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 8.71 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 133 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.91 kB 0 B
build/block-library/editor.css 8.91 kB 0 B
build/block-library/index.js 147 kB 0 B
build/block-library/style-rtl.css 8.1 kB 0 B
build/block-library/style.css 8.1 kB 0 B
build/block-library/theme-rtl.css 792 B 0 B
build/block-library/theme.css 793 B 0 B
build/block-serialization-default-parser/index.js 1.87 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48 kB 0 B
build/components/index.js 171 kB 0 B
build/components/style-rtl.css 15.3 kB 0 B
build/components/style.css 15.3 kB 0 B
build/compose/index.js 9.9 kB 0 B
build/core-data/index.js 14.8 kB 0 B
build/data-controls/index.js 821 B 0 B
build/data/index.js 8.74 kB 0 B
build/date/index.js 11.2 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.92 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.43 kB 0 B
build/edit-post/style.css 6.42 kB 0 B
build/edit-site/index.js 23.2 kB 0 B
build/edit-site/style-rtl.css 3.98 kB 0 B
build/edit-site/style.css 3.98 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.16 kB 0 B
build/edit-widgets/style.css 3.16 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/index.js 42.5 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.62 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.86 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 713 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.1 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.83 kB 0 B
build/reusable-blocks/index.js 3.05 kB 0 B
build/rich-text/index.js 13.3 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested and it all now works as I would expect.

The only thing holding off an approval is the curiosity about "not in edit-site" comment, which I see you have ping'd @nosolosw for a response on that.

} else {
// STEP 3 - OTHERWISE, ADD STYLES
//
// If we are in a block editor context, but not in edit-site,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I wonder why "not in edit-site" and if we are missing anything? 🤔

@oandregal
Copy link
Member

oandregal commented Nov 16, 2020

👋 I've taken a look at this and want to share my thoughts. There are two issues here:

Blocks that depend on the .block-editor class being present

I've searched for the block-editor selector and these blocks depend on it being present:

  • archives
  • categories
  • latest-posts
  • more
  • rss
  • shortcode
  • tag-cloud

There are another set of blocks that depend on the selector through the dependency on the block-editor/components/plain-text:

  • file
  • HTML
  • shortcode

Most of these block's CSS hasn't been updated for 2 years, so it looks like a case where the new editors introduced didn't test that these blocks worked properly. Although the change here seems to work, I'm unsure if we should make all editor use the .block-editor selector or whether it's preferable that we remove that selector from the blocks that have it?

I'm largely unfamiliar with how these editors came to be and the role of this selector so pinging a few folks for context @noahtallen @vindl for FSE/edit-site, @youknowriad @jasmussen for post-editor, @draganescu @talldan for widgets screen. It looks like the navigation doesn't have this issue because none of the affected blocks are allowed here.

Global styles in the site editor

These styles are present in the site editor, only that they're generated client-side (look for the embedded stylesheet with id global-styles-inline-css). The reason these are loaded differently for the site editor is that the user can update the styles through the Global Styles sidebar, so we need a way to consolidate the styles (core, theme, and user) upon user changes.

By looking at the issue description I'm unsure what's the issue? Would you mind expanding on this?

@Copons
Copy link
Contributor Author

Copons commented Nov 16, 2020

Thanks for looking at this @nosolosw!

Global styles in the site editor

These styles are present in the site editor, only that they're generated client-side (look for the embedded stylesheet with id global-styles-inline-css). The reason these are loaded differently for the site editor is that the user can update the styles through the Global Styles sidebar, so we need a way to consolidate the styles (core, theme, and user) upon user changes.

By looking at the issue description I'm unsure what's the issue? Would you mind expanding on this?

It's I think specific to the HTML block which loads the preview in an iframe.

That iframe needs the editor styles to properly display the content, and it grabs them from ( 'core/block-editor' ).settings.styles.
Since we are not registering the Global Styles settings in the Site Editor, the HTML preview iframe can't get them, and falls back to whatever default (see in the screenshots: before is incorrectly serif).

If I'm understanding the code correctly, on update Global Styles directly modifies <style id="global-styles-inline-css"> in the DOM.
I see that it's also supposed to updateSettings, but the action diff in the Redux dev tools is empty. 🤔
I guess we should make sure the inline style and the Redux settings are kept in sync.

@jasmussen
Copy link
Contributor

Interestingly, I just ran into this and fixed it on my end: Automattic/block-experiments#156

@oandregal
Copy link
Member

It seems that we have two different issues here:

  1. The use of .block-editor selector in some blocks => we either have to add that class to all editors or remove the class from blocks.

#27063 attempts to remove the class from the blocks that have it, as they shouldn't depend on a specific editor selector. I'd appreciate wide testing and feedback there.

  1. There's some blocks that don't work well with how we generate the styles dynamically in edit-site.

HTML preview (uses iframe) has this issue. In a conversation with @jorgefilipecosta he brought up that other blocks may have issues as well (classic, shortcode, etc). I haven't confirmed but this needs investigation.

To fix this second issue we need to update how we generate the stylesheet client-side for edit-site: we need to be able to inject (and update upon user changes) our client-side generated stylesheet within the settings.styles. We also need to respect all the existing styles.

I'm going to start working on this (unless someone has already started).

@oandregal
Copy link
Member

PR for second issue at #27065

@oandregal oandregal deleted the fix/edit-site-missing-global-styles branch November 19, 2020 16:01
@oandregal
Copy link
Member

Both PRs have been merged so this should be fixed. Thanks for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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