Conversation
|
@fabiankaegy will add some time on our calendar to chat further. But at least wanted to get a draft started of a robust README on the Theme itself as team members may use it on a one off and want to know how it works. Also saw we hadn't updated the dependencies for awhile and the project was still using JS. Anywho feel free to add comments and guidance but wanted to get the wheels turning to get your thoughts. |
fabiankaegy
left a comment
There was a problem hiding this comment.
Thanks so much for getting this started @pdavies88 🚀
Left a bunch of notes inline and also pinned @darylldoyle & @claytoncollie for additional thoughts :)
There was a problem hiding this comment.
Big fan of making this change official in the theme here 👍
Something I always needed to do in projects though is add the following to the .eslintrc file:
{
"parser": "@typescript-eslint/parser",
"extends": ["@10up/eslint-config/wordpress"],
"plugins": ["@typescript-eslint"],
"rules": {}
}There was a problem hiding this comment.
Do you really think there is value in having the example block back in here?
Part of the reason why I removed it was that I ended up seeing it nor removed on client projects. And since we have the scaffold:block command in the theme here I always found it nicer / easier to just have that present.
There was a problem hiding this comment.
I'd actually be in favor or removing this because it's also something we always have to remove and folks forget that :/
There was a problem hiding this comment.
I know it goes against what I was saying above, but I would like to keep these here.
Section styles is an under utilized feature in my opinion so I want users of this theme to wonder about it and start to learn more about them.
See https://ignitewp.10uplabs.com/block-styling-and-css/#section-styles-in-ignite
| - `register_theme_blocks()` scans `dist/blocks/*/block.json` and calls `register_block_type_from_metadata()` for each found block. | ||
| - `allowed_block_types_all` filter extends allowed blocks with theme blocks. | ||
| - `enqueue_theme_block_styles()` scans `dist/blocks/autoenqueue/**/*.css` and registers/enqueues style/script for each block with assets info from `GetAssetInfo`. | ||
| - includes block render filter for TenUp navigation block (removes ARIA roles for fallback markup compatibility). |
There was a problem hiding this comment.
Not sure I understand what's meant by this
| - `enqueue_theme_block_styles()` scans `dist/blocks/autoenqueue/**/*.css` and registers/enqueues style/script for each block with assets info from `GetAssetInfo`. | ||
| - includes block render filter for TenUp navigation block (removes ARIA roles for fallback markup compatibility). | ||
|
|
||
| ### 6.2 Blocks directory structure |
There was a problem hiding this comment.
Let's mention the scaffold command here
|
|
||
| ### 7.2 Theme script inclusion | ||
|
|
||
| - `src/ThemeCore::theme_setup()` calls `add_theme_support('editor-styles')` and `add_editor_style('/dist/css/frontend.css')`. |
There was a problem hiding this comment.
This is only for editor stuff. From the heading I would have expected something about the Asset trait from the framework and how to enqueue things
|
|
||
| ## 16. Further notes | ||
|
|
||
| - This theme is designed around a 10up block theme pattern (colors, spacing, utilities, templates). |
There was a problem hiding this comment.
Not sure I understand what is meant by this
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
Co-authored-by: Fabian Kägy <mail@fabian-kaegy.de>
There was a problem hiding this comment.
@pdavies88 I don't agree with having smaller readme's throughout the codebase that can easily become out of date. I think this either should be a CLAUDE.md file (still run into same out of date issue) or place all of this into the root README/CLAUDE.md files.
There was a problem hiding this comment.
@pdavies88 I agree with @fabiankaegy that we should not provide an example block here as people do not take ownership and make sure it gets deleted. We have so many resources to scaffold this ou either with a cli or the documentation at gutenberg.10up.com
There was a problem hiding this comment.
@pdavies88 same thing here. Either a CLAUDE.md file or we consilidate into the main readme
| "blocksDir": "./blocks" | ||
| } | ||
| }, | ||
| "optionalDependencies": { |
There was a problem hiding this comment.
@fabiankaegy Does 10up toolkit already load these?
|
|
||
| ## 2. Requirements | ||
|
|
||
| - PHP >= 8.2 |
There was a problem hiding this comment.
@fabiankaegy we should bump this to 8.4 IMO cc @pdavies88
| ## 2. Requirements | ||
|
|
||
| - PHP >= 8.2 | ||
| - Node >= 18 |
There was a problem hiding this comment.
@fabiankaegy we should bump this to v24 IMO cc @pdavies88
|
|
||
| - PHP >= 8.2 | ||
| - Node >= 18 | ||
| - Composer |
There was a problem hiding this comment.
@pdavies88 if we are putting versions, this should be Composer 2
| npm run build | ||
| ``` | ||
|
|
||
| ### 4.2 Local development |
There was a problem hiding this comment.
@pdavies88 this totally ignore PHP commands in composer.json. we should treat both PHP and JS/CSS as equal partners
| ### 4.2 Local development | ||
|
|
||
| - `npm run watch` (or `npm start`) → 10up-toolkit watch with HMR/hot-refresh | ||
| - `composer exec phpcs -- --standard=WordPress` → PHP lint |
| ### 4.3 Common gotchas | ||
|
|
||
| - Always build `dist/` after changing source assets (`assets/`, `blocks/`, `src/`). | ||
| - If using a check-in strategy for `dist/`, ensure CI runs `npm run build` before release. |
There was a problem hiding this comment.
@pdavies88 this is very nuanced and vague. I think we remove. this assumes too much about deployment which can be varied
| - Always build `dist/` after changing source assets (`assets/`, `blocks/`, `src/`). | ||
| - If using a check-in strategy for `dist/`, ensure CI runs `npm run build` before release. | ||
| - `theme.json` is the source of truth for editor styles, palette, and layout. Adjust there for global styles. | ||
| - Some references may still use legacy naming in older helper docs; prefer 10up-block-theme naming in code changes. |
There was a problem hiding this comment.
@pdavies88 can we remove this? I though we caught all of the instances.
| ### 4.1 Quick start (repeat for copy/paste) | ||
|
|
||
| ```bash | ||
| cd /path/to/themes/10up-block-theme |
There was a problem hiding this comment.
@pdavies88 you should not have to do this operation. Both NPM and Composer work from the wp-content directory to install and build all assets, not just the theme
|
|
||
| 1. Clone the repository: | ||
| ```bash | ||
| git clone <repo-url> 10up-block-theme |
There was a problem hiding this comment.
@pdavies88 I think we are getting too specific here for a theme only setup, and not sticking to the original intent of the repo which covers a large majority of use cases. Most projects use this entire scaffold, not just the theme cc @fabiankaegy
There was a problem hiding this comment.
@pdavies88 sorry, i am just not seeing this is the theme readme. I think this is going to throw people off the main path. I appreciate that some projects only use the theme, but a vast majority will use the entire scaffold. Fabian has spent a lot of time making things run from the wp-content directory link install and built and scaffold commands and now we are bypassing all of that work. I think it is fine to explain this theme, but then say to reference the main readme for scaffold and install instructions. cc @fabiankaegy
|
|
||
| --- | ||
|
|
||
| ## 5. Core bootstrapping path |
There was a problem hiding this comment.
@pdavies88 Similar to what we are finding with AGENTS.md and CLAUDE.md file, this information can go stale over time. I think all of this function reference should be removed from here in favor of docblocks on the class or methods it relates to.
| - `npm install` | ||
| - `npm run watch` | ||
| - `npm run build` | ||
| - `composer exec phpcs -- --standard=WordPress` |
There was a problem hiding this comment.
| - `composer exec phpcs -- --standard=WordPress` | |
| - `composer run lint` |
|
|
||
| --- | ||
|
|
||
| ## 16. Further notes |
There was a problem hiding this comment.
@pdavies88 I think we can remove this section. feels redundant after all that is said eariler
|
|
||
| --- | ||
|
|
||
| ## 15. Helpful commands summary |
There was a problem hiding this comment.
@pdavies88 I think we can remove this section. This is the third instance where we are listing the same commands.
|
|
||
| --- | ||
|
|
||
| ## 14. Key entrypoints quick map |
|
|
||
| --- | ||
|
|
||
| ## 13. Maintenance tasks |
There was a problem hiding this comment.
@pdavies88 can we consolidate these commands into the earlier list at the start of this file?
Description of the Change
For the 10up-block-theme it performs the following:
Closes: N/A
How to test the Change
Can be pulled into a local WP build and run and function as normal
Changelog Entry
Credits
Props @pdavies88
Checklist: