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 theme that uses the latest block style system + block templates #39

Closed
wants to merge 9 commits into from

Conversation

jffng
Copy link
Collaborator

@jffng jffng commented May 27, 2020

This PR adds a minimal theme example whose styles rely the latest iteration of the Global Styles mechanism.

The theme supplies one block template (just an index.html that renders the post content), an experimental-theme.json, along with the necessary functions.php file.

Screen Shot 2020-05-29 at 4 15 05 PM

(Design inspired by Beatriz Fialho)


.home .entry-content > .wp-block-cover {
height: 100vh;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A good exercise here would be to try to understand why every style rule here is needed, and whether it's something that should be absorbed by block attributes that are missing or something expected to rely on CSS.

@jffng jffng marked this pull request as ready for review June 2, 2020 20:44
"color": "var(--wp--preset--color--valencia)"
}
},
"core/heading/h1": {

Choose a reason for hiding this comment

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

I'm not sure if I see how this does anything. Removing it does not change anything relating to the output or color of an H1 tag. @jffng Would you be able to help me understand what this is doing? 🙏

Screen Shot 2020-06-19 at 1 48 08 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I haven't been able to get this part to work either (both here and in another theme I tried this with).

Copy link
Collaborator Author

@jffng jffng Jun 22, 2020

Choose a reason for hiding this comment

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

I think the API changed since I first opened this PR. Just pushed some changes that should address it, tested on GB 8.3.

"value": "#FFDE69"
},
{
"slug": "navy",

Choose a reason for hiding this comment

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

@jffng Could you help me understand why a slug is needed here? If the slug is navy and that is then used as part of a CSS variable name, I feel like that might defeat the purpose of a CSS variable. To me, the purpose of a variable is that the value van vary. If the value (navy or 022384) is used in the name, you can't change the value of that variable without the name becoming redundant. Once I change the value of --wp--preset--color--navy to something other than #022384, the word "navy" no longer describes the value.

That makes me wonder if slugs should be completely removed, and automatically generated to create a predictable standard which themes could rely on their stylesheets. They could be named based on their order, for example:

--wp--preset--color--one
--wp--preset--color--two
--wp--preset--color--three

If I am totally misunderstanding something, please let me know!

Copy link
Member

Choose a reason for hiding this comment

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

Totally agree. I’ve gone with primary, secondary, etc in the Go theme (for this same reason).

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also had an issue recently where a popular plugin overwrote the primary and secondary color values, which is often not what we want?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, for now the slug is only used to generate a CSS variable name, so something more generic would make sense.

The exact names here relate back to this old issue around whether or not to standardize custom color names: WordPress/gutenberg#7553

Some standardization does make sense, and the advent of Global Styles may be a good time to start.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johnstonphilip thanks for the review!

Re: slugs, that's a fair point. In the case of colors, they could be something generic. I named them as such because it made the example more straightforward to keep my naming straight between the editor, the theme's stylesheet, and the theme config.

That makes me wonder if slugs should be completely removed, and automatically generated to create a predictable standard which themes could rely on their stylesheets

I think I agree. Right now there is no relationship between presets and styles in the theme config. It could make sense if there were a set of common CSS variables that Gutenberg used to predictably style core blocks, and themes could override.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

popular plugin overwrote the primary and secondary color values

@carolinan out of curiosity, what do you mean? The theme would provide some CSS variables, and an unrelated plugin would overwrite them because they happened to be named the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, like this: Yoast/wordpress-seo#15345

Fix styles implementation.
@jffng
Copy link
Collaborator Author

jffng commented Oct 21, 2020

This was a good way to explore an early implementation of the theme.json, but I'm closing for now.

@jffng jffng closed this Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants