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

Update platform docs intro #61341

Merged
merged 4 commits into from
Dec 11, 2024
Merged

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented May 3, 2024

closes #67048

What?

Updates the intro.md with some critical things that were missing for a workable build to be produced when following the guide. Among those:

  • Recommends the React variant as part of npm create vite@latest as without this JSX won’t work by default and react dependencies won’t be installed. The React variant also installs some other nice defaults like eslint with react related plugins.
  • Details that @wordpress/block-library expects IS_GUTENBERG_PLUGIN and how to provide it as without this a runtime error makes for a broken app.
  • Adds a missing style import @wordpress/block-editor/build-style/content.css
  • Specifies the styles prop of BlockCanvas with imported content styles as without this various things won’t be styled in the canvas.

Less important changes:

  • Adds ?raw param to style imports to avoid Vite ever trying to process them.
  • Instructs that registerCoreBlocks() be put in src/main.jsx so it won’t execute during HMR and cause errors (they are non-fatal but they sure clutter up the console).
  • Specifies to update src/App.jsx instead of index.jsx because it’s what’ll exist when using the React variant and it also means the createRoot/render bits can be removed from the code sample.

Along with that there were a some minor edits to either fix typos or try to make it read a little better.

Why?

To hopefully approach the expectation that following this guide will produce a working example in 10 minutes. From the docs homepage:
image
You’ll probably go well over 10 minutes if you have to discover how to fix all the gotchas you'll hit by following the guide as is.

How?

I think this is mostly covered in What. I may add some review comments.

Testing Instructions

cd platform-docs
npm install
npm run start

Ensure it all looks good.

Copy link

github-actions bot commented May 3, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @aks30498.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: aks30498.

Co-authored-by: stokesman <presstoke@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@stokesman stokesman marked this pull request as draft May 3, 2024 01:41
@michalczaplinski
Copy link
Contributor

👋 @stokesman I've closed my PR (#60087) in favor of yours here as your PR is more comprehensive.

Since #61486 has been merged, it's probably unnecessary to include the point about process.env.IS_GUTENBERG_PLUGIN. I haven't tested it to confirm, though.

if (
! registeredBlockTypes.length ||
! registeredBlockTypes.some((blockType) => blockType.name.startsWith('core/'))
) registerCoreBlocks();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a simpler way to do this. We're trying to teach people how to build a block editor, we should try to keep things as simple as possible. For instance is there something like ! isHMR && registerCoreBlocks() instead of using the block package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn’t see a simple way to branch for HMR but could have missed something. Reregistering the blocks was not any fatal issue but it polluted the console quite a bit. Now that I’ve tested with the latest @wordpress packages I’m not seeing console messages output for blocks and I don’t know why. Still, there is one that gets output for the core/footnote format. The simplest way I thought of to avoid this is instructing that the registerCoreBlocks call be made in src/main.jsx.

${ blocksCommonStyles }
${ blocksStyles }
${ blocksEditorStyles }
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

While we should do this for "content styles", some of these styles shouldn't be injected into the iframe and others should be injected inside and outside the iframe:

  • components (in both places because of placeholders and buttons)
  • block-editor/style style shouldn't be loaded in the iframe IMO (if something is not working we should fix it in gutenberg), instead it should be loaded in the app directly (so direct import here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I’d meant to revisit this and get it right.

  • (if something is not working we should fix it in gutenberg)

Agreed. I can’t quite recall what style issues I’d noted. Perhaps the default block appender was not getting styled but since #66630 that doesn’t appear to be an issue.

onInput={setBlocks}
>
<>
<style>{ styles }</style>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's avoid this and just do importfor the two stylesheets that need to load here. We should clarify what things are "content" styles and what things are "UI styles". I think we should add a paragraph to explain this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. This style tag is gone. This reason it was this way was because I’d added the ?raw param to the their import path to tell Vite not to process them and due to that Vite won’t automatically inject them. I can’t remember if there was any issue with Vite processing them or if I just thought it was better dev performance. Now instead of doing an import for those files I’ve instructed that they be added to index.html (the vite-ignore attribute will also keep them from being processed).

I know it’s not quite as simple that way but I think it may help express the difference with these UI styles versus the content styles. I also added a note hoping to help explain it, not sure if it suffices.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, especially the content styles part.

@stokesman stokesman marked this pull request as ready for review December 10, 2024 23:50
@stokesman stokesman added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Developer Documentation Documentation for developers labels Dec 10, 2024
import blockEditorContentStyles from "@wordpress/block-editor/build-style/content.css?raw";

// Default styles that are needed for the core blocks.
import blocksCommonStyles from "@wordpress/block-library/build-style/common.css?raw";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized this one was extra, it is @import‘d in the block library’s style (the one on the next line).

*/ }
<BlockCanvas
height="500px"
styles={ [
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there's any performance value in putting a stable array here (maybe define this array outside the component)

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

So I didn't actually try the code, but this looks good to me.

@youknowriad
Copy link
Contributor

I think this closes #67048

@stokesman stokesman merged commit 1a8771b into trunk Dec 11, 2024
61 of 62 checks passed
@stokesman stokesman deleted the update/platform-docs-intro-for-reality branch December 11, 2024 16:05
@stokesman stokesman changed the title Update platform docs intro so following it actually works Update platform docs intro Dec 11, 2024
@github-actions github-actions bot added this to the Gutenberg 20.0 milestone Dec 11, 2024
yogeshbhutkar pushed a commit to yogeshbhutkar/gutenberg that referenced this pull request Dec 18, 2024
…61341)

Unlinked contributors: aks30498.

Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: michalczaplinski <czapla@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg Framework: Clarify and simplify how to load content styles
3 participants