Add a block based theme option #238

fabiankaegy · 2024-09-30T18:43:12Z

Description of the Change

TBD – Very much work in progress

Closes #

How to test the Change

Changelog Entry

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability
Developer - Non-functional update

Credits

Props @username, @username2, ...

fabiankaegy · 2024-09-30T18:44:56Z

CC: @10up/editorial-engineering Would love to get your all's input on this :)

Also CC: @darylldoyle we can use this new scaffold to update how we are doing things. So if we want to introduce larger coding style / structural changes this is a time where we can include stuff like that :)

fabiankaegy · 2024-09-30T18:45:48Z

themes/10up-block-theme/webpack.config.js

This is no longer needed now that Toolkit 6.3 is out. We can safely remove this :)

fabiankaegy · 2024-09-30T18:46:47Z

themes/10up-block-theme/theme.json

+			"defaultDuotone": false,
+			"defaultGradients": false,
+			"defaultPalette": false,
+			"palette": [


Pretty much all the tokens in here are inspired by UI Kit. I don't think we should be as opinionated in the scaffold.

Though I would like us to ship stuff like the section styles for color theming.

fabiankaegy · 2024-09-30T18:47:28Z

themes/10up-block-theme/stylelint.config.js

We should double check why this file needed to be created and patch it upstream in toolkit

Might give some context: https://fueled10up.slack.com/archives/C035LFXLQH0/p1692947406604129

@fabiankaegy

fabiankaegy · 2024-09-30T18:47:45Z

themes/10up-block-theme/screenshot.png

Needs to be removed

fabiankaegy · 2024-09-30T18:48:27Z

themes/10up-block-theme/phpcs.xml

+<ruleset name="10up PHPCS">
+	<description>10up PHPCS extended.</description>
+	<exclude-pattern>dist/</exclude-pattern>
+	<exclude-pattern>plugins/*</exclude-pattern>


Why is this here?

Also do we even need this file if there is one at the root of the project?

This looks incorrect, based on the fact that it's excluding plugins/*, but, I do think there's some benefit to this, especially if we move ahead with something like 10up/10up-toolkit#424, as it would mean any themes/plugins pulled in via that command would have valid PHPCS configs, even if pulled into legacy codebases.

fabiankaegy · 2024-09-30T18:49:26Z

themes/10up-block-theme/includes/utility.php

+ * }
+ * @return void
+ */
+function wp_enqueue_block_view_script( $block_name, $args = array() ) {


If we want to include this Utility function we should change the name spacing to ensure this doesn't break if WP actually ships this utility.

fabiankaegy · 2024-09-30T18:49:57Z

themes/10up-block-theme/includes/overrides.php

+ *
+ * @return void
+ */
+function setup() {


Is all of this still relevant today? Let's audit!

fabiankaegy · 2024-09-30T18:50:22Z

themes/10up-block-theme/includes/core.php

+	wp_register_style( 'ct', false ); // phpcs:ignore
+	wp_enqueue_style( 'ct' );
+	wp_add_inline_style( 'ct', 'head{--ct-is-problematic:solid;--ct-is-affected:dashed;--ct-notify:#0bce6b;--ct-warn:#ffa400;--ct-error:#ff4e42}head,head [rel=stylesheet],head script,head script:not([src])[async],head script:not([src])[defer],head script~meta[http-equiv=content-security-policy],head style,head>meta[charset]:not(:nth-child(-n+5)){display:block}head [rel=stylesheet],head script,head script~meta[http-equiv=content-security-policy],head style,head title,head>meta[charset]:not(:nth-child(-n+5)){margin:5px;padding:5px;border-width:5px;background-color:#fff;color:#333}head ::before,head script,head style{font:16px/1.5 monospace,monospace;display:block}head ::before{font-weight:700}head link[rel=stylesheet],head script[src]{border-style:var(--ct-is-problematic);border-color:var(--ct-warn)}head script[src]::before{content:"[Blocking Script – " attr(src) "]"}head link[rel=stylesheet]::before{content:"[Blocking Stylesheet – " attr(href) "]"}head script:not(:empty),head style:not(:empty){max-height:5em;overflow:auto;background-color:#ffd;white-space:pre;border-color:var(--ct-notify);border-style:var(--ct-is-problematic)}head script:not(:empty)::before{content:"[Inline Script] "}head style:not(:empty)::before{content:"[Inline Style] "}head script:not(:empty)~title,head script[src]:not([async]):not([defer]):not([type=module])~title{display:block;border-style:var(--ct-is-affected);border-color:var(--ct-error)}head script:not(:empty)~title::before,head script[src]:not([async]):not([defer]):not([type=module])~title::before{content:"[<title> blocked by JS] "}head [rel=stylesheet]:not([media=print]):not(.ct)~script,head style:not(:empty)~script{border-style:var(--ct-is-affected);border-color:var(--ct-warn)}head [rel=stylesheet]:not([media=print]):not(.ct)~script::before,head style:not(:empty)~script::before{content:"[JS blocked by CSS – " attr(src) "]"}head script[src][src][async][defer]{display:block;border-style:var(--ct-is-problematic);border-color:var(--ct-warn)}head script[src][src][async][defer]::before{content:"[async and defer is redundant: prefer defer – " attr(src) "]"}head script:not([src])[async],head script:not([src])[defer]{border-style:var(--ct-is-problematic);border-color:var(--ct-warn)}head script:not([src])[async]::before{content:"The async attribute is redundant on inline scripts"}head script:not([src])[defer]::before{content:"The defer attribute is redundant on inline scripts"}head [rel=stylesheet][href^="//"],head [rel=stylesheet][href^=http],head script[src][src][src^="//"],head script[src][src][src^=http]{border-style:var(--ct-is-problematic);border-color:var(--ct-error)}head script[src][src][src^="//"]::before,head script[src][src][src^=http]::before{content:"[Third Party Blocking Script – " attr(src) "]"}head [rel=stylesheet][href^="//"]::before,head [rel=stylesheet][href^=http]::before{content:"[Third Party Blocking Stylesheet – " attr(href) "]"}head script~meta[http-equiv=content-security-policy]{border-style:var(--ct-is-problematic);border-color:var(--ct-error)}head script~meta[http-equiv=content-security-policy]::before{content:"[Meta CSP defined after JS]"}head>meta[charset]:not(:nth-child(-n+5)){border-style:var(--ct-is-problematic);border-color:var(--ct-warn)}head>meta[charset]:not(:nth-child(-n+5))::before{content:"[Charset should appear as early as possible]"}link[rel=stylesheet].ct,link[rel=stylesheet][media=print],script[async],script[defer],script[type=module],style.ct{display:none}' );


@dainemawer are we actually using this?

fabiankaegy · 2024-09-30T18:50:43Z

themes/10up-block-theme/includes/core.php

+function scrollbar_detection() {
+	echo '<script>window.addEventListener("DOMContentLoaded",()=>{const t=()=>window.innerWidth-document.body.clientWidth;const e=()=>{document.documentElement.style.setProperty("--wp--custom--scrollbar-width",`${t()}px`)};e();window.addEventListener("resize",e);});</script>' . "\n";


Not sure we want this in the scaffold.

Here is more context

CC @dmtrmrv

fabiankaegy · 2024-09-30T18:51:38Z

themes/10up-block-theme/includes/core.php

+function register_all_icons() {
+	if ( ! function_exists( '\UIKitCore\Helpers\register_icons' ) ) {
+		return;
+	}
+
+	$icon_paths = glob( TENUP_BLOCK_THEME_DIST_PATH . 'svg/*.svg' );
+	$icons      = array_map(
+		function ( $icon_path ) {
+			$icon_name = preg_replace( '#\..*$#', '', basename( $icon_path ) );
+
+			return new \UIKitCore\Icon(
+				$icon_name,
+				ucwords( str_replace( '-', ' ', $icon_name ) ),
+				$icon_path
+			);
+		},
+		$icon_paths
+	);
+
+	\UIKitCore\Helpers\register_icons(
+		[
+			'name'  => 'tenup',
+			'label' => 'Theme Icons',
+			'icons' => $icons,
+		]
+	);
+}


Very torn if we want this here.

I think every single project we build needs to have a solution for icons. But this makes it dependent on UI Kit. So not sure what the best option is.

@fabiankaegy

I'd keep it. I feel like this makes sense - if we are using UI Kit - "it should just work" and your early escape resolves if not.

I would add more detail in comments regarding the dependency on UI Kit.

If we find helpful, someone can add an issue to utilize a more agnostic approach option within this function.

joesnellpdx · 2024-09-30T22:50:17Z

@fabiankaegy

I'll also share with the FEE Best Practices WG - as some of the suggested and similar changes here have been discussed in that WG

colinswinney · 2024-10-02T14:03:27Z

themes/10up-block-theme/assets/css/base/link.css

+ *
+ * using :where to prevent the specificity increase of using :not
+ */
+a:where(:not(.components-external-link, :has(> img:only-child), .wp-element-button)) {


I would like to see a similar file for form elements. I've seen a few projects that don't scope their input/select etc selectors and it affects the editor placeholder components. I've used the following with success to prevent bleeding into those.

// base/forms.css &:where(:not(.components-placeholder)) { input {...} }

colinswinney · 2024-10-02T14:09:23Z

themes/10up-block-theme/assets/css/base/reset.css

@@ -0,0 +1,85 @@
+/*


I've been adding this to my resets as well to ensure alt tags remain readable if the image doesn't load. Sometimes you see folks put font-size: 0; or line-height: 0 on figure/div tags to avoid that small bit of white space that can occur, this protects against that situation.

img { font-size: 1rem; font-style: italic; line-height: 1.5; }

colinswinney · 2024-10-02T14:15:50Z

themes/10up-block-theme/assets/css/mixins/margin-collapse.css

@@ -0,0 +1,14 @@
+@define-mixin margin-collapse {


I would recommend calling this margin-trim to better reflect what its doing.

colinswinney · 2024-10-02T14:19:03Z

themes/10up-block-theme/assets/css/utilities/visually-hidden.css

@@ -0,0 +1,10 @@
+.visually-hidden {


I would add this to the mixins and then call that here. It's very handy to have available in your styles vs only as a utility class.

@define-mixin visually-hidden { border: 0; clip: rect(0, 0, 0, 0); height: 1px; margin: -1px; overflow: hidden; padding: 0; position: absolute; width: 1px; } .visually-hidden { @mixin visually-hidden; }

I do like that pattern but I also worry that it leads to a lot of duplicated / bloated CSS 🤔

colinswinney · 2024-10-02T14:30:38Z

themes/10up-block-theme/assets/css/base/layout.css

+	/* stylelint-disable-next-line length-zero-no-unit */
+	--site-header-top-offset: var(--wp-admin--admin-bar--height, 0px);
+
+	&.has-pinned-header {


Is has-pinned-header a class generated by WP in any way? This seems a bit unnecessary unless there is some mechanism by which that would be commonly added in addition to the --header-height var.

Whoops, yeah these should be removed. They are project specific

darylldoyle · 2024-10-02T16:14:39Z

themes/10up-block-theme/includes/core.php

+	$n = function ( $function_name ) {
+		return __NAMESPACE__ . "\\$function_name";
+	};


thought (non-blocking): It seems like this is unused now (🤞) Let's remove it anyway and use the full namespace as we are below in all-the-places

Yeah this should be removed. I used the full namespace so we get better IDE integration (Props @tobeycodes )

Thank you! <3 IDE integration

darylldoyle · 2024-10-02T16:20:14Z

themes/10up-block-theme/functions.php

+// Run the setup functions.
+TenupBlockTheme\Core\setup();
+TenupBlockTheme\Blocks\setup();
+TenupBlockTheme\TemplateTags\setup();


thought (non-blocking): Conceptually, I have an issue with this approach, and it's not something we only do here. I don't understand why we mix OOP and functional code for no real reason. IMO, we should stick to one approach or another unless functions are being provided for templating reasons (in which case, we wouldn't need to setup() or init() any hooks; they should be encapsulated in classes.

I'm all ear on what we can do to avoid this :)

I agree with this but perhaps we should apply this to the whole scaffold in a separate PR. It'll be a big change

darylldoyle · 2024-10-02T16:24:11Z

themes/10up-block-theme/phpcs.xml

+<ruleset name="10up PHPCS">
+	<description>10up PHPCS extended.</description>
+	<exclude-pattern>dist/</exclude-pattern>
+	<exclude-pattern>plugins/*</exclude-pattern>


This looks incorrect, based on the fact that it's excluding plugins/*, but, I do think there's some benefit to this, especially if we move ahead with something like 10up/10up-toolkit#424, as it would mean any themes/plugins pulled in via that command would have valid PHPCS configs, even if pulled into legacy codebases.

tobeycodes

@fabiankaegy I saw this PR in my notifications and thought I'd leave some comments. Feel free to disregard anything that is not relevant. It looks great! Excited for block themes

tobeycodes · 2024-10-05T16:14:23Z

themes/10up-block-theme/.lintstagedrc.json

+    "*.js": [
+      "10up-toolkit lint-js"
+    ],
+    "*.jsx": [
+      "10up-toolkit lint-js"
+    ],


question: should we add ts and tsx here?

tobeycodes · 2024-10-05T16:17:34Z

themes/10up-block-theme/.lintstagedrc.json

+      "10up-toolkit lint-js"
+    ],
+    "*.php": [
+      "./vendor/bin/phpcs --extensions=php --warning-severity=8 -s"


suggestion: these arguments should in the config itself to prevent inconsistencies with IDE integration and lintstagedrc

<fileextensions> <extension>php</extension> </fileextensions>  <arg name="warning-severity" value="8"/>  <arg name="s"/>

tobeycodes · 2024-10-05T16:18:44Z

themes/10up-block-theme/.eslintrc

thought: we should add this to the .editorconfig file and make sure it's formatted like the other rc files.

https://github.com/10up/wp-scaffold/blob/trunk/.editorconfig#L10

tobeycodes · 2024-10-05T16:19:37Z

themes/10up-block-theme/.nvmrc

question: can we bump this to the latest LTS?

tobeycodes · 2024-10-05T16:20:43Z

themes/10up-block-theme/assets/css/base/layout.css

question: why do we need the backwards compatibility? if a new project is started they won't be referencing the old variables and existing projects are not backwards porting future wp-scaffold updates

tobeycodes · 2024-10-05T16:23:42Z

themes/10up-block-theme/functions.php

+// Run the setup functions.
+TenupBlockTheme\Core\setup();
+TenupBlockTheme\Blocks\setup();
+TenupBlockTheme\TemplateTags\setup();


I agree with this but perhaps we should apply this to the whole scaffold in a separate PR. It'll be a big change

tobeycodes · 2024-10-05T16:27:36Z

themes/10up-block-theme/includes/blocks.php

question: should we be using a cache for performance (e.g. https://github.com/roots/bedrock-autoloader/blob/master/src/Autoloader.php) and perhaps RecursiveDirectoryIterator instead of glob?

tobeycodes · 2024-10-05T16:28:50Z

themes/10up-block-theme/includes/blocks.php

+			$block_folder = dirname( $filename );
+			$block        = register_block_type_from_metadata( $block_folder );
+
+			add_filter(


request: can this go outside of the foreach loop? here we are calling add_filter for every block registered rather than once with all the blocks to add

tobeycodes · 2024-10-05T16:30:31Z

themes/10up-block-theme/includes/blocks.php

+		if ( ! file_exists( $asset_file ) ) {
+			$asset_file = require $asset_file;


thought: it appears there is a logical issue here. if the file does not exist, you can't require it

tobeycodes · 2024-10-05T16:31:15Z

themes/10up-block-theme/includes/core.php

+	$n = function ( $function_name ) {
+		return __NAMESPACE__ . "\\$function_name";
+	};


Thank you! <3 IDE integration

add block based theme as a starter

3e5cd5b

fabiankaegy added enhancement New feature or request Internal Review labels Sep 30, 2024

fabiankaegy self-assigned this Sep 30, 2024

fabiankaegy commented Sep 30, 2024

View reviewed changes

themes/10up-block-theme/webpack.config.js

Copy link

Member Author

fabiankaegy Sep 30, 2024

Choose a reason for hiding this comment

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

This is no longer needed now that Toolkit 6.3 is out. We can safely remove this :)

fabiankaegy commented Sep 30, 2024

View reviewed changes

themes/10up-block-theme/screenshot.png

Copy link

Member Author

fabiankaegy Sep 30, 2024

Choose a reason for hiding this comment

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

Needs to be removed

fabiankaegy commented Sep 30, 2024

View reviewed changes

colinswinney reviewed Oct 2, 2024

View reviewed changes

darylldoyle reviewed Oct 2, 2024

View reviewed changes

tobeycodes reviewed Oct 5, 2024

View reviewed changes

		function scrollbar_detection() {
		echo '<script>window.addEventListener("DOMContentLoaded",()=>{const t=()=>window.innerWidth-document.body.clientWidth;const e=()=>{document.documentElement.style.setProperty("--wp--custom--scrollbar-width",`${t()}px`)};e();window.addEventListener("resize",e);});</script>' . "\n";

		if ( ! file_exists( $asset_file ) ) {
		$asset_file = require $asset_file;

Add a block based theme option #238

Are you sure you want to change the base?

Add a block based theme option #238

Conversation

fabiankaegy commented Sep 30, 2024

Description of the Change

How to test the Change

Changelog Entry

Credits

fabiankaegy commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joesnellpdx commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobeycodes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment