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

Add the colophon block #29

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

TremiDkhar
Copy link

Changes proposed in this Pull Request

  • Add the colophon block

Testing instructions

  • Edit the site using the FSE Site Editor.
  • Search and add the Colophon block.
  • The user can change the font-size, font-family, font-weight, etc. of the block.

Mentions #8

@TremiDkhar TremiDkhar changed the title Add the colophon block (#8) Add the colophon block Apr 24, 2024
Copy link
Contributor

@georgestephanis georgestephanis left a comment

Choose a reason for hiding this comment

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

A couple passing thoughts -- some aren't really necessary, but more for code clarity and simplification. Let's remove some stock boilerplate files if we're not actually using them.

@@ -0,0 +1,5 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's worth including the editor.scss if we're not using it for anything. May as well purge it to keep clutter files down?

@@ -0,0 +1,6 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not sure it's worth including this if it's not doing anything. We can keep the plugin simpler with less junk clutter files by not including them if they're not used.

Comment on lines +28 to +29
"editorStyle": "file:./index.css",
"style": "file:./style-index.css",
Copy link
Contributor

Choose a reason for hiding this comment

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

These can be purged as we're not actually using them.

Comment on lines +14 to +23
"fontSize": true,
"lineHeight": true,
"__experimentalFontFamily": true,
"__experimentalFontStyle": true,
"__experimentalFontWeight": true,
"__experimentalLetterSpacing": true,
"__experimentalTextTransform": true,
"__experimentalDefaultControls": {
"fontSize": true
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these are all managed via the block attributes, which is why we don't have styles implementing them. 👍

"editorStyle": "file:./index.css",
"style": "file:./style-index.css",
"render": "file:./render.php"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know block context can be used to set a block where it can only be used in a post context or the like --

https://developer.wordpress.org/block-editor/reference-guides/block-api/block-context/

Is there an option we can set so that the block is only usable in a FSE / page template context, so we're not cluttering up the block options when someone's writing a post?

Copy link
Contributor

Choose a reason for hiding this comment

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

Blocks meant for use in FSE themes are called Theme Blocks, and I think this would be the way to do it:

https://github.com/WordPress/gutenberg/blob/e706d67c7482e752a4adaae3e0b0449234e19902/packages/block-library/src/navigation/block.json#L6

@@ -0,0 +1,3 @@
<div <?php echo wp_kses_data( get_block_wrapper_attributes() ); ?>>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this wp_kses_data() call is necessary here -- as it is designed to be sanitizing not escaping, (so focused on content not context), and doesn't seem to be used by the Gutenberg team when doing similar tasks:

https://github.com/WordPress/gutenberg/blob/e706d67c7482e752a4adaae3e0b0449234e19902/docs/getting-started/tutorial.md?plain=1#L685-L687

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.

2 participants