-
Notifications
You must be signed in to change notification settings - Fork 50
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 a block based theme option #238
base: trunk
Are you sure you want to change the base?
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.
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 :)
"defaultDuotone": false, | ||
"defaultGradients": false, | ||
"defaultPalette": false, | ||
"palette": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should double check why this file needed to be created and patch it upstream in toolkit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might give some context: https://fueled10up.slack.com/archives/C035LFXLQH0/p1692947406604129
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be removed
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we want this in the scaffold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend calling this margin-trim
to better reflect what its doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! :)
@@ -0,0 +1,10 @@ | |||
.visually-hidden { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all ear on what we can do to avoid this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: can we bump this to the latest LTS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, ...