Add a block based theme option #238
Conversation
|
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 :) |
There was a problem hiding this comment.
This is no longer needed now that Toolkit 6.3 is out. We can safely remove this :)
| "defaultDuotone": false, | ||
| "defaultGradients": false, | ||
| "defaultPalette": false, | ||
| "palette": [ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should double check why this file needed to be created and patch it upstream in toolkit
There was a problem hiding this comment.
Might give some context: https://fueled10up.slack.com/archives/C035LFXLQH0/p1692947406604129
| <ruleset name="10up PHPCS"> | ||
| <description>10up PHPCS extended.</description> | ||
| <exclude-pattern>dist/</exclude-pattern> | ||
| <exclude-pattern>plugins/*</exclude-pattern> |
There was a problem hiding this comment.
Why is this here?
Also do we even need this file if there is one at the root of the project?
There was a problem hiding this comment.
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.
| * } | ||
| * @return void | ||
| */ | ||
| function wp_enqueue_block_view_script( $block_name, $args = array() ) { |
There was a problem hiding this comment.
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.
| * | ||
| * @return void | ||
| */ | ||
| function setup() { |
There was a problem hiding this comment.
Is all of this still relevant today? Let's audit!
| 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}' ); |
There was a problem hiding this comment.
@dainemawer are we actually using this?
| 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"; |
There was a problem hiding this comment.
Not sure we want this in the scaffold.
There was a problem hiding this comment.
| 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, | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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 |
| * | ||
| * using :where to prevent the specificity increase of using :not | ||
| */ | ||
| a:where(:not(.components-external-link, :has(> img:only-child), .wp-element-button)) { |
There was a problem hiding this comment.
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 {...}
}| @@ -0,0 +1,85 @@ | |||
| /* | |||
There was a problem hiding this comment.
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;
}| @@ -0,0 +1,14 @@ | |||
| @define-mixin margin-collapse { | |||
There was a problem hiding this comment.
I would recommend calling this margin-trim to better reflect what its doing.
| @@ -0,0 +1,10 @@ | |||
| .visually-hidden { | |||
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
I do like that pattern but I also worry that it leads to a lot of duplicated / bloated CSS 🤔
| /* stylelint-disable-next-line length-zero-no-unit */ | ||
| --site-header-top-offset: var(--wp-admin--admin-bar--height, 0px); | ||
|
|
||
| &.has-pinned-header { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Whoops, yeah these should be removed. They are project specific
| $n = function ( $function_name ) { | ||
| return __NAMESPACE__ . "\\$function_name"; | ||
| }; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah this should be removed. I used the full namespace so we get better IDE integration (Props @tobeycodes )
There was a problem hiding this comment.
Thank you! <3 IDE integration
| // Run the setup functions. | ||
| TenupBlockTheme\Core\setup(); | ||
| TenupBlockTheme\Blocks\setup(); | ||
| TenupBlockTheme\TemplateTags\setup(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm all ear on what we can do to avoid this :)
There was a problem hiding this comment.
I agree with this but perhaps we should apply this to the whole scaffold in a separate PR. It'll be a big change
| <ruleset name="10up PHPCS"> | ||
| <description>10up PHPCS extended.</description> | ||
| <exclude-pattern>dist/</exclude-pattern> | ||
| <exclude-pattern>plugins/*</exclude-pattern> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
| "*.js": [ | ||
| "10up-toolkit lint-js" | ||
| ], | ||
| "*.jsx": [ | ||
| "10up-toolkit lint-js" | ||
| ], |
There was a problem hiding this comment.
question: should we add ts and tsx here?
| "10up-toolkit lint-js" | ||
| ], | ||
| "*.php": [ | ||
| "./vendor/bin/phpcs --extensions=php --warning-severity=8 -s" |
There was a problem hiding this comment.
suggestion: these arguments should in the config itself to prevent inconsistencies with IDE integration and lintstagedrc
<fileextensions>
<extension>php</extension>
</fileextensions>
<!-- Set the severity level for warnings -->
<arg name="warning-severity" value="8"/>
<!-- Show sniff codes in output (-s option) -->
<arg name="s"/>
themes/10up-block-theme/.eslintrc
Outdated
There was a problem hiding this comment.
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
themes/10up-block-theme/.nvmrc
Outdated
There was a problem hiding this comment.
question: can we bump this to the latest LTS?
There was a problem hiding this comment.
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
| // Run the setup functions. | ||
| TenupBlockTheme\Core\setup(); | ||
| TenupBlockTheme\Blocks\setup(); | ||
| TenupBlockTheme\TemplateTags\setup(); |
There was a problem hiding this comment.
I agree with this but perhaps we should apply this to the whole scaffold in a separate PR. It'll be a big change
There was a problem hiding this comment.
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?
| $block_folder = dirname( $filename ); | ||
| $block = register_block_type_from_metadata( $block_folder ); | ||
|
|
||
| add_filter( |
There was a problem hiding this comment.
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
| if ( ! file_exists( $asset_file ) ) { | ||
| $asset_file = require $asset_file; |
There was a problem hiding this comment.
thought: it appears there is a logical issue here. if the file does not exist, you can't require it
| $n = function ( $function_name ) { | ||
| return __NAMESPACE__ . "\\$function_name"; | ||
| }; |
There was a problem hiding this comment.
Thank you! <3 IDE integration
Description of the Change
TBD – Very much work in progress
Closes #
How to test the Change
Changelog Entry
Credits
Props @username, @username2, ...