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

Try: Adding a barebones block-based version of the Twenty Twenty-One Theme #57

Merged
merged 9 commits into from
Oct 22, 2020

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Oct 21, 2020

An attempt to kick off development for the block-based version of the Twenty Twenty-One theme.

This generally replicates the look & feel of the original theme.
It's meant to be a starting point for more development.

As of today it includes:

  • Base scaffolding styles (CSS only, no Sass)
  • experimental-theme.json, populated with defaults from Twenty Twenty-One
  • Front Page, Index, Page, and Single templates.
  • Header, Footer Block Template parts
  • Block Patterns imported from Twenty Twenty-One
  • Block Styles imported from Twenty Twenty-One

Known bugs:

  • It shows a "Template Part Not Found" message when it's loaded from a subdirectory.
  • Separator Blocks are doubled up in the editor.
  • Post/Page Titles need styles.
  • Block Styles still need some style rules from Twenty Twenty-One:
    • Latest Posts: Dividers
    • Latest Posts: Borders
    • Editor styles for Social Links: Dark gray color

Screenshots

Front End Full Site Editor
gutenberg test_-(1) gutenberg test_wp-admin_admin php_page=gutenberg-edit-site-(1)

@kjellr kjellr added the block-based theme A theme using HTML templates label Oct 21, 2020
@kjellr kjellr self-assigned this Oct 21, 2020
@kjellr
Copy link
Collaborator Author

kjellr commented Oct 21, 2020

One other caveat for testing: It looks like folks will need to test against Gutenberg's master branch for now. 9.2 appears to be doing something weird to the rendering of the templates — repeating classes and causing some weird bugs on the front end.

@carolinan
Copy link
Collaborator

carolinan commented Oct 21, 2020

I really wish there was even a minimum of testing of how a PR affects FSE before its merged in to Gutenberg.

nags experimental does not mean it should be broken every two weeks.

If we can't test, without having to troubleshoot two weeks worth of commits, then we can't help move FSE forward.

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

One other caveat for testing: It looks like folks will need to test against Gutenberg's master branch for now. 9.2 appears to be doing something weird to the rendering of the templates — repeating classes and causing some weird bugs on the front end.

Yeah, this is the front-end running Gutenberg on master:

Screen Shot 2020-10-21 at 4 05 01 PM

And this is the front-end with 9.2 installed:

Screen Shot 2020-10-21 at 4 15 20 PM

Seems like some kind of parsing error going on:
Screen Shot 2020-10-21 at 4 15 39 PM

@nosolosw do you know if this will be resolved in a minor release?

@oandregal
Copy link
Member

I hope so! There's another bug (regression for the code block) that people also want to be fixed. Not sure about the timing but should be soon. Some more context: https://wordpress.slack.com/archives/C02QB2JS7/p1603349339442700?thread_ts=1603311591.436000&cid=C02QB2JS7

@mtias
Copy link
Member

mtias commented Oct 22, 2020

Having a theme target would also help start building E2E tests to prevent front-end regressions. Excited to see this!

@@ -0,0 +1,19 @@
<!-- wp:spacer {"height":70} -->
<div style="height:70px" aria-hidden="true" class="wp-block-spacer"></div>
Copy link
Member

Choose a reason for hiding this comment

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

Would you say this should be better expressed as top padding on the footer template part if we had such a control?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would certainly be one way to do it — it would come in handy for the header too. At the moment, those template parts are truly just empty wrappers with no settings (other than alignment, I guess). But I could see them being a little more full-featured: padding, background color, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

An additional, traditional .site-footer class name with styles works too ;)

Copy link
Member

Choose a reason for hiding this comment

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

Template parts (at least these two) should be more "robust" — output a semantic tag, have style attributes, and yes, a class :)

It might be nice to conceptualize template parts as site- fragments. So by default template parts would output a class of site-{template-part-name}.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 22, 2020

It shows a "Template Part Not Found" message when it's loaded from a subdirectory.

If anyone can help figure out why this happens, that would be great. All the other themes in this repo work well if you load them from within wp-content/themes/theme-experiments/themename, but this one throws that error. It works fine if it's loaded from wp-content/themes/themename. 🤔

@scruffian
Copy link
Collaborator

If anyone can help figure out why this happens, that would be great. All the other themes in this repo work well if you load them from within wp-content/themes/theme-experiments/themename, but this one throws that error. It works fine if it's loaded from wp-content/themes/themename. 🤔

Fixed here: WordPress/gutenberg#26391

.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > h6:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > ul:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > ol:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > pre:not(.has-background) {
Copy link
Collaborator

@scruffian scruffian Oct 22, 2020

Choose a reason for hiding this comment

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

could this be simplified to?
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > *:not(.has-background)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so unfortunately — we don't want it to add additional padding to a Group or Cover block for example. The rules are only targeting text-based blocks.

Copy link
Member

Choose a reason for hiding this comment

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

This is also a good candidate to figure out how to declare it better in theme.json @jorgefilipecosta @nosolosw

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this comes up quite a lot. Could we add a class to all text based blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, themes frequently want to add padding to only-text-based blocks when they're inside of fullwide columns too. A single class would come in handy for that.

Copy link
Member

Choose a reason for hiding this comment

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

We don't know what a text based block is, though

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add that to Gutenberg though..

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggested it here: WordPress/gutenberg#26407

Copy link
Member

Choose a reason for hiding this comment

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

This is also a good candidate to figure out how to declare it better in theme.json @jorgefilipecosta @nosolosw

Theme.json is very far from being able to select a selector like this. In order to do so we would need:

  • allow theme.json selectors specific to block styles.
    • If we have a block style selector, should the selector be special and accept styles or should it accept settings too? E.g.: in columns with a specific block style, only certain colors are available in the palette?
  • Allow nesting of styles e.g.: target group paragraphs inside a column. We don't have nesting, and it seems a theme needs nesting as we can see it is used multiple times in this theme. Should we add a nesting mechanism? If we add nesting, should we have general nesting, or should we also have the option of direct descendent like CSS does with ">"?
  • The equivalent of nth-child(2n). Should we have something like that in theme.json? If yes we have two options:
    • Implement an equivalent of where on the run time, we can verify if a block matches the sector and even allow the application of custom settings.
    • Output directly the nth-child(2n) in CSS, so the styles work well but don't do any runtime verification if the block matches a selector or not. This approach would be generic. We would probably have some system to allow any or most of the selectors CSS allows. It may be problematic on mobile as we would probably not be able to apply the styles correctly there.
  • The equivalent of not(.has-background). This is a very custom selector. I guess we would not be able to have a runtime way of detecting it (unless we query the dom). Probably the way to address this would be to have a way to use any CSS selector. As referred before, that way would only support styles and not settings.
  • Allow outputting of properties we don't have a UI control for yet. Currently, if we don't recognize a theme.json style property, e.g.: marginTop or zIndex, we discard it. I guess we would make theme.json more useful, at least until we don't have more controls if we simply outputted the styles e.g: zIndex: 2-> z-index: 2.

Copy link
Member

Choose a reason for hiding this comment

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

We should not be attempting to replicate that exact CSS code but understand what is it trying to accomplish. Why is the margin necessary? Why the z-index? Can it be absorbed in the way columns lay out content?


.wp-block-image.alignfull img,
.wp-block-image.alignwide img {
height: auto;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this should live in the image block itself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I thought it did, but it doesn't seem to. 😕

Copy link
Collaborator

Choose a reason for hiding this comment

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

Image blocks don't seem to stretch to full width:
Screenshot 2020-10-22 at 15 21 02

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's odd — I haven't been able to reproduce. 🤔 I just modified that code because it needs to apply to all images, not just those that are alignwide/alignfull. Mind giving it another try?

--------------------------------------------------------------*/

.wp-block-media-text.is-style-twentytwentyone-border {
border: 3px solid var(--wp--custom--color--border);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we make this border apply to everything with this class?:

.is-style-twentytwentyone-border {
	border: 3px solid var(--wp--custom--color--border);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It only applies to a few blocks, and a couple of them need to be slightly different. The image block for instance needs to be applied to .is-style-twentytwentyone-border img instead of just .is-style-twentytwentyone-border. And the group block needs some extra padding, so it will still need a .wp-block-group.is-style-twentytwentyone-border rule anyway.

We could declare these all in one place (or define a variable for 3px solid var(--wp--custom--color--border) at least), but I don't have a strong preference either way.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 22, 2020

I just pushed a few minor updates:

  • Updating the main README to include this theme in the list.
  • Adding a theme screenshot.

From the discussion above, it sounds like this is in a decent spot to begin building from. If we have other issues, we can open and explore those as separate issues.

Would someone mind giving this one more review before merge?

Copy link
Collaborator

@jffng jffng left a comment

Choose a reason for hiding this comment

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

I think this is a great start @kjellr and sets us up nicely to iterate on the block-based version of TT1 🚢

2. Visit the Gutenberg -> Experiments settings page and check the "Enable Full Site Editing" box. Save your changes.
3. Click the 'Code' button on [the main Theme Experiments Repository GitHub page](https://github.com/wordpress/theme-experiments).
4. Unzip the resulting download.
4. Create a new zip file containing only the "twentytwentyone-blocks" folder.
Copy link
Collaborator

@jffng jffng Oct 22, 2020

Choose a reason for hiding this comment

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

This is cumbersome but I get why it has to be this way for now: because @scruffian's template resolution fix did not make it in 9.2.1, you'll still get a "Template Part Not Found" error if the theme exists in a subdirectory (e.g. if you clone the theme experiments repo into your themes directory)

But I don't think this should be a blocker for merging the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cool, we can revise this README once that's merged in. 👍

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 22, 2020

Thanks for the reviews, everyone. When further issues come up, we can open them as individual issues in this repository. 🚢

@kjellr kjellr merged commit e3ff68d into master Oct 22, 2020
@kjellr kjellr deleted the add/twentytwentyone-blocks branch October 22, 2020 19:06
]
},
"typography": {
"customFontSize": true,
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't need to set this value to true as it is the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, you're probably right. I'll remove.

.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > h6:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > ul:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > ol:not(.has-background),
.is-style-twentytwentyone-columns-overlap .wp-block-column:nth-child(2n) > pre:not(.has-background) {
Copy link
Member

Choose a reason for hiding this comment

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

This is also a good candidate to figure out how to declare it better in theme.json @jorgefilipecosta @nosolosw

Theme.json is very far from being able to select a selector like this. In order to do so we would need:

  • allow theme.json selectors specific to block styles.
    • If we have a block style selector, should the selector be special and accept styles or should it accept settings too? E.g.: in columns with a specific block style, only certain colors are available in the palette?
  • Allow nesting of styles e.g.: target group paragraphs inside a column. We don't have nesting, and it seems a theme needs nesting as we can see it is used multiple times in this theme. Should we add a nesting mechanism? If we add nesting, should we have general nesting, or should we also have the option of direct descendent like CSS does with ">"?
  • The equivalent of nth-child(2n). Should we have something like that in theme.json? If yes we have two options:
    • Implement an equivalent of where on the run time, we can verify if a block matches the sector and even allow the application of custom settings.
    • Output directly the nth-child(2n) in CSS, so the styles work well but don't do any runtime verification if the block matches a selector or not. This approach would be generic. We would probably have some system to allow any or most of the selectors CSS allows. It may be problematic on mobile as we would probably not be able to apply the styles correctly there.
  • The equivalent of not(.has-background). This is a very custom selector. I guess we would not be able to have a runtime way of detecting it (unless we query the dom). Probably the way to address this would be to have a way to use any CSS selector. As referred before, that way would only support styles and not settings.
  • Allow outputting of properties we don't have a UI control for yet. Currently, if we don't recognize a theme.json style property, e.g.: marginTop or zIndex, we discard it. I guess we would make theme.json more useful, at least until we don't have more controls if we simply outputted the styles e.g: zIndex: 2-> z-index: 2.

--------------------------------------------------------------*/

h1.wp-block-site-title {
font-size: calc(1px * var(--wp--preset--font-size--normal));
Copy link
Member

Choose a reason for hiding this comment

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

Could we use theme.json for the font-szie rule?

add_theme_support( 'responsive-embeds' );

// Add support for custom line height controls.
add_theme_support( 'custom-line-height' );
Copy link
Member

Choose a reason for hiding this comment

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

I guess this supports could now be active with theme.json? Would you be able to confirm @nosolosw ?

max-width: 360px;
}

@media screen and (min-width: 1290px) {
Copy link
Member

Choose a reason for hiding this comment

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

@mtias, @nosolosw Should the v1 of global styles have an equivalent to media queries where we can have different styles depending on screen resolution, or for v1 it is not something we should include?

Copy link
Member

Choose a reason for hiding this comment

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

Not included. Once Gutenberg supports custom values for attributes in responsive breakpoints we can look into it.

"font-primary": "-apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Oxygen-Sans, Ubuntu, Cantarell, 'Helvetica Neue', sans-serif",
"font-weight-light": 300,
"color": {
"primary": "var(--wp--preset--color--dark-gray)",
Copy link
Member

Choose a reason for hiding this comment

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

The user can change the presets and consequently change the value of the primary is it expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now yes — that's fine. I initially created these custom variables just to make it easier to migrate the existing color rules over from the original version of Twenty Twenty-One. We should be able to remove these and map to the exact preset palette colors instead in a followup PR.

"core/paragraph": {
"styles": {
"color": {
"text": "var(--wp--custom--color--primary)",
Copy link
Member

Choose a reason for hiding this comment

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

Would applying "text": "var(--wp--custom--color--primary)", at the global level instead of for each text block also work?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the intention is to prepare the different colors this way in order to help developers or advanced users to change the values?
Similar to how the CSS variables can be used in the PHP version of the theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would applying "text": "var(--wp--custom--color--primary)", at the global level instead of for each text block also work?

It should — I believe when I tried it, it didn't work though so I did this instead. I'd have to dig back into it and double check.

"text": "var(--wp--custom--color--primary)"
},
"typography": {
"fontSize": "var(--wp--preset--font-size--small)",
Copy link
Member

Choose a reason for hiding this comment

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

It seems like writing styles in theme.json is much more verbose when compared to CSS, when we need to apply the same style to multiple selectors. Should we have a mechanism in theme.json to target multiple selectors? cc: @mtias, @nosolosw

* This theme does not use a hard-coded <title> tag in the document head,
* WordPress will provide it for us.
*/
add_theme_support( 'title-tag' );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not needed for FSE themes, the title is already added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
block-based theme A theme using HTML templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants