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 a block based theme option #238

Draft
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Draft

Conversation

fabiankaegy
Copy link
Member

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 fabiankaegy added enhancement New feature or request Internal Review labels Sep 30, 2024
@fabiankaegy fabiankaegy self-assigned this Sep 30, 2024
@fabiankaegy
Copy link
Member Author

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 :)

Copy link
Member Author

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": [
Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

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>
Copy link
Member Author

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?

Copy link
Collaborator

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() ) {
Copy link
Member Author

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() {
Copy link
Member Author

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!

Comment on lines +187 to +189
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}' );
Copy link
Member Author

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?

Comment on lines +162 to +163
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";
Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +113 to +139
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,
]
);
}
Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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
Copy link
Contributor

@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

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

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 @@
/*

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 {

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.

Copy link
Member Author

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 {

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;
}

Copy link
Member Author

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 {

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.

Copy link
Member Author

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

Comment on lines +18 to +20
$n = function ( $function_name ) {
return __NAMESPACE__ . "\\$function_name";
};
Copy link
Collaborator

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

Copy link
Member Author

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 )

Copy link
Member

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

Comment on lines +37 to +40
// Run the setup functions.
TenupBlockTheme\Core\setup();
TenupBlockTheme\Blocks\setup();
TenupBlockTheme\TemplateTags\setup();
Copy link
Collaborator

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.

Copy link
Member Author

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 :)

Copy link
Member

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>
Copy link
Collaborator

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.

Copy link
Member

@tobeycodes tobeycodes left a 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

Comment on lines +5 to +10
"*.js": [
"10up-toolkit lint-js"
],
"*.jsx": [
"10up-toolkit lint-js"
],
Copy link
Member

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"
Copy link
Member

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"/>

Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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

Comment on lines +37 to +40
// Run the setup functions.
TenupBlockTheme\Core\setup();
TenupBlockTheme\Blocks\setup();
TenupBlockTheme\TemplateTags\setup();
Copy link
Member

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

Copy link
Member

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(
Copy link
Member

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

Comment on lines +57 to +58
if ( ! file_exists( $asset_file ) ) {
$asset_file = require $asset_file;
Copy link
Member

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

Comment on lines +18 to +20
$n = function ( $function_name ) {
return __NAMESPACE__ . "\\$function_name";
};
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Internal Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants