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

feat(cxl-lumo-styles): add editor styles #428

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

anoblet
Copy link
Collaborator

@anoblet anoblet commented Aug 8, 2024

Copy link

github-actions bot commented Aug 8, 2024

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 44.75 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 15.04 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 29.23 KB (0%)
packages/cxl-ui/pkg/dist-web/vendor.js 157.96 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-institute.js, packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 291.42 KB (0%)

@anoblet anoblet marked this pull request as ready for review August 8, 2024 14:53
package.json Outdated Show resolved Hide resolved
@@ -13,6 +13,7 @@
"@vaadin/icon": "^23.3.7",
"@vaadin/polymer-legacy-adapter": "^23.3.7",
"@vaadin/vaadin-lumo-styles": "^23.3.7",
"@vaadin/vaadin-themable-mixin": "^23.3.7"
"@vaadin/vaadin-themable-mixin": "^23.3.7",
"sass-embedded": "^1.77.8"
Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@anoblet anoblet Aug 9, 2024

Choose a reason for hiding this comment

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

There's not much to reuse, it would be mostly copy/paste. sass-embedded provides a performance benefit. We don't need to interpolate templates, or add features which are already baked in. The sass-embedded CLI is pretty straight forward.

Choose a reason for hiding this comment

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

I think we should not compile it in aybolit but just provide a SASS file which is later compiled in themes: https://github.com/conversionxl/cxl-wpstarter/blob/0a44eaf3e29137218d2959a0d672d39ad9ff6b6d/packages/institute-theme/package.json#L23

Thoughts @lkraav ?

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

I included generated stylesheet and editor shows styles closer to the frontend now 👍

But it still needs tweaks:

  • headings font sizes are different then real sizes,
  • styles leak to the whole edit page, they should only apply to block editor.
Screenshot 2024-08-22 at 13 11 18 Screenshot 2024-08-22 at 13 12 32

Tips: https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-support/#editor-styles
Editor stylesheet is enqueued here: https://github.com/conversionxl/cxl-wpstarter/blob/master/packages/wp-theme-lib/src/BlockEditor.php#L33

@pawelkmpt
Copy link

@anoblet follow up

@anoblet
Copy link
Collaborator Author

anoblet commented Sep 1, 2024

headings font sizes are different then real sizes,

Our typography depends on https://github.com/vaadin/web-components/blob/main/packages/vaadin-lumo-styles/typography.js which is written in JS. If this file isn't loaded, most of our typography styles won't work. I've copy and pasted it, so if we were able to load those as well, headings should work.

image

styles leak to the whole edit page, they should only apply to block editor.

Our styles aren't properly being scoped to just the block editor, and are being overwritten by reset.css.

image

When looking at the documentation, I see this is needed:

add_theme_support( 'editor-styles' );

I've tried pasting this into various parts of BlockEditor.php without any luck. The documentation says to use add_editor_style( 'style-editor.css' );, though I see that we use this instead:

add_action( 'enqueue_block_editor_assets', static function(): void {
    wp_enqueue_style( 'cxl-editor', get_stylesheet_directory_uri() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css', [], Utils::get_formatted_filemtime( get_stylesheet_directory() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css' ) );
} );

Things we need to do moving forward:

  1. Find a way to sync with vaadin-lumo-styles, or if we feel like these styles won't change very often, copy/paste them into cxl-lumo-styles.
  2. Figure out why add_theme_support( 'editor-styles' ); isn't working.

Edit:

Replacing:

add_action( 'enqueue_block_editor_assets', static function(): void {
    wp_enqueue_style( 'cxl-editor', get_stylesheet_directory_uri() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css', [], Utils::get_formatted_filemtime( get_stylesheet_directory() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css' ) );
} );

With:

add_theme_support( 'editor-styles' );
add_editor_style( get_stylesheet_directory_uri() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css', [], Utils::get_formatted_filemtime( get_stylesheet_directory() . Utils::CXL_WP_THEME_LIB_PATH . '/editor.css' ) );

Works. All that's left is the vaadin-lumo-styles sync.

@lkraav
Copy link

lkraav commented Sep 1, 2024

Avoid any «sync» strategy. Goal has always been to create another editor-styles JS bundle in Aybolit (not sure if current frontend bundle makes sense, but can try), and load it into post editor (now finally) iframe.

This is mid-discussion link into a long 🧵 https://cxlworld.slack.com/archives/C01JABH8AHX/p1671525151733279?thread_ts=1622046838.000700&cid=C01JABH8AHX lmk if reading through this helps clarify past present and future?

@anoblet
Copy link
Collaborator Author

anoblet commented Sep 2, 2024

Thanks for the background :) I'm still a bit confused as to how far the IFrame initiative has gotten? When I go to edit a page on live, or WP Starter, the editor doesn't use one. From what I've read, an IFrame is the eventual goal, though currently there are still limitations(blocks need to support API v3, custom fields aren't supported.)

The documentation @pawelkmpt linked(https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-support/#editor-styles) shows that the classic editor uses an IFrame, though the block editor does not. This is why we need to use add_theme_support( 'editor-styles' ); which scopes the editor styles automatically. As far as I can tell add_editor_style only supports CSS.

In order to get the vaadin-lumo-style tokens loaded, we would need to copy/paste them into our own stylesheet, or include that module specifically using JS. For example adding this to the top of the header page:

<script type="module" src="path/to/vaadin-lumo-styles/all-imports.js" />

I'm trying to wrap my mind around a JS bundle solution, though we would need to import vaadin-lumo-styles, import our own styles, then use the Vaadin registerGlobalStyles utility.

@pawelkmpt
Copy link

pawelkmpt commented Sep 4, 2024

Avoid any «sync» strategy. Goal has always been to create another editor-styles JS bundle in Aybolit (not sure if current frontend bundle makes sense, but can try), and load it into post editor (now finally) iframe.

In order to get the vaadin-lumo-style tokens loaded, we would need to copy/paste them into our own stylesheet, or include that module specifically using JS. For example adding this to the top of the header page:

<script type="module" src="path/to/vaadin-lumo-styles/all-imports.js" />

I'm trying to wrap my mind around a JS bundle solution, though we would need to import vaadin-lumo-styles, import our own styles, then use the Vaadin registerGlobalStyles utility.

@lkraav @anoblet Is this the correct summary of our situation?

Vaadin & Lumo

  • Typography styles come as JS file, so we can't just include it in SCSS
  • We can create custom editor module which would include that file
  • Custom editor module would be loaded in the backend providing necessary styles
  • Should we wrap all of these styles with .editor-styles-wrapper { ... } like WordPress automatically does for custom editor styles?

WordPress

  • We'd load CSS file compiled out of SCSS typography overrides

@lkraav
Copy link

lkraav commented Sep 4, 2024

When I go to edit a page on live, or WP Starter, the editor doesn't use one.

https://make.wordpress.org/core/2023/07/18/miscellaneous-editor-changes-in-wordpress-6-3/#post-editor-iframed says "From WordPress 6.3 on, the post editor will be iframed if all registered blocks have a Block API version 3 or higher. Adding version 3 support means that the block should work inside an iframe, though the block may still be rendered outside the iframe if not all blocks support version 3."

Are we loading maybe (unintentionally) some legacy blocks?

Is this the correct summary of our situation?

I believe all of your checklist is solved at once by just figuring out how to enable iframe'd post editor, and load a web components bundle into it.

@anoblet
Copy link
Collaborator Author

anoblet commented Sep 4, 2024

@pawelkmpt All of this looks correct.

Should we wrap all of these styles with .editor-styles-wrapper { ... } like WordPress automatically does for custom editor styles?

This I'm unsure of. For the time being it seems to be the way to go until we can figure out why our instance doesn't use IFrames. Once we are using IFrames, we should remove this wrapper.

@pawelkmpt
Copy link

Are we loading maybe (unintentionally) some legacy blocks?

I don't know what makes block version 3 but we have custom block which have not been edited for years and it already might be an issue.

@lkraav
Copy link

lkraav commented Sep 5, 2024

Are we loading maybe (unintentionally) some legacy blocks?

I don't know what makes block version 3 but we have custom block which have not been edited for years and it already might be an issue.

Yep our own legacy garbage is the biggest risk. Lmk when y'all know.

@pawelkmpt
Copy link

@anoblet I got to the conclusion the easiest way to get necessary lumo-styles from JS files into CSS is to copy-paste them and I did so.

While dist:wp-block-editor works nicely, yarn build throws an error. Can you please take it over from me and solve it?

yarn build
yarn run v1.22.22
$ rimraf packages/**/styles/**/*.js && yarn build-styling
$ node packages/sass-render/bin/sass-render.js -s "packages/*/scss/**/*.scss"
Processing packages/core/scss/button-base.scss
Processing packages/core/scss/checkbox-base.scss
Processing packages/cxl-lumo-styles/scss/badge.scss
Processing packages/cxl-lumo-styles/scss/block-editor/spacing.scss
Processing packages/cxl-lumo-styles/scss/block-editor/style.scss
Processing packages/cxl-lumo-styles/scss/block-editor/sizing.scss
Processing packages/cxl-lumo-styles/scss/block-editor/color.scss
Processing packages/cxl-lumo-styles/scss/block-editor/typography.scss
Processing packages/cxl-lumo-styles/scss/color.scss
Processing packages/cxl-lumo-styles/scss/global.scss
Processing packages/cxl-lumo-styles/scss/gravity-forms.scss
Processing packages/core/scss/range-base.scss
Processing packages/core/scss/switch-base.scss
Processing packages/core/scss/progress-base.scss
Processing packages/cxl-lumo-styles/scss/loading.scss
Processing packages/cxl-lumo-styles/scss/icons.scss
Processing packages/cxl-lumo-styles/scss/themes/cxl-checkout-details.scss
Processing packages/cxl-lumo-styles/scss/themes/cxl-tabs-slider.scss
Processing packages/cxl-lumo-styles/scss/themes/cxl-accordion-card.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-accordion-panel.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-accordion.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-button.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-context-menu-list-box.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-context-menu-item.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-context-menu-overlay.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-details.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-icon.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-horizontal-layout.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-menu-bar-button.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-menu-bar.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-dialog-overlay.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-notification-card.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-notification-container.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-overlay.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-tab.scss
Processing packages/cxl-lumo-styles/scss/themes/vaadin-tabs.scss
Processing packages/cxl-lumo-styles/scss/typography.scss
Processing packages/cxl-ui/scss/cxl-base-card.scss
Processing packages/cxl-lumo-styles/scss/wp-block-editor.scss
Processing packages/cxl-ui/scss/cxl-card.scss
Processing packages/cxl-ui/scss/cxl-course-card.scss
Processing packages/cxl-ui/scss/cxl-app-layout.scss
Processing packages/cxl-ui/scss/cxl-dashboard-header.scss
Processing packages/cxl-ui/scss/cxl-dashboard-notification.scss
Processing packages/cxl-ui/scss/cxl-dashboard-section.scss
Processing packages/cxl-ui/scss/cxl-featured-course-card.scss
Processing packages/cxl-ui/scss/cxl-dashboard-team-stats.scss
Processing packages/cxl-ui/scss/cxl-dashboard-team-header.scss
Processing packages/cxl-ui/scss/cxl-featured-image.scss
Processing packages/cxl-ui/scss/cxl-credential.scss
Processing packages/cxl-ui/scss/cxl-certificate-header.scss
Processing packages/cxl-ui/scss/cxl-jw-player/cxl-jw-player-transcript-shadow.scss
Processing packages/cxl-ui/scss/cxl-jw-player/cxl-jw-player-shadow.scss
Processing packages/cxl-ui/scss/cxl-like-or-dislike.scss
Processing packages/cxl-ui/scss/cxl-marketing-nav.scss
Processing packages/cxl-ui/scss/cxl-notification-card.scss
Processing packages/cxl-ui/scss/cxl-inline-notification.scss
Processing packages/cxl-ui/scss/cxl-save-favorite.scss
Processing packages/cxl-ui/scss/cxl-section.scss
Processing packages/cxl-ui/scss/cxl-light-card.scss
Processing packages/cxl-ui/scss/global/cxl-card.scss
Processing packages/cxl-ui/scss/global/cxl-app-layout.scss
Processing packages/cxl-ui/scss/cxl-time.scss
Processing packages/cxl-ui/scss/global/cxl-certificate-header.scss
Processing packages/cxl-ui/scss/global/cxl-dashboard-section.scss
Processing packages/cxl-ui/scss/global/cxl-course-card.scss
Processing packages/cxl-ui/scss/global/cxl-checkout-details.scss
Processing packages/cxl-ui/scss/cxl-stats.scss
Processing packages/cxl-ui/scss/global/cxl-course-dialog.scss
Processing packages/cxl-ui/scss/global/cxl-dashboard-team-header.scss
Processing packages/cxl-ui/scss/global/cxl-jw-player/cxl-jw-player-chapter-navigation.scss
Processing packages/cxl-ui/scss/global/cxl-jw-player/cxl-jw-player-nextup.scss
Processing packages/cxl-ui/scss/global/cxl-jw-player/cxl-jw-player-transcript.scss
Processing packages/cxl-ui/scss/global/cxl-marketing-nav.scss
Processing packages/cxl-ui/scss/global/cxl-playbook-accordion.scss
Processing packages/cxl-ui/scss/global/cxl-jw-player/cxl-jw-player.scss
Processing packages/cxl-ui/scss/global/cxl-section.scss
Processing packages/cxl-ui/scss/global/cxl-stats.scss
Processing packages/cxl-ui/scss/global/cxl-vaadin-accordion.scss
[Error: ENOENT: no such file or directory, open 'packages/cxl-lumo-styles/src/styles/block-editor/sizing-css.js'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: 'packages/cxl-lumo-styles/src/styles/block-editor/sizing-css.js'
}
error Command failed with exit code 255.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 255.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@anoblet
Copy link
Collaborator Author

anoblet commented Oct 10, 2024

While dist:wp-block-editor works nicely, yarn build throws an error.

This should be fixed.

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.

3 participants