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

Blockbase: Add a way to create a theme #4862

Closed
wants to merge 2 commits into from
Closed

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented Oct 18, 2021

Changes proposed in this Pull Request:

This is a proposal for a mechanism to create Blockbase child themes.

The way it works is like this:

  1. Switch to a Blockbase theme that you want to clone
  2. Make any changes you want to see to the block templates or theme.json settings. You can do this using Global Styles or using the customizer
  3. Go to the "Create theme" in the menu, and fill in the details:

Screenshot 2021-10-18 at 20 47 41

4. When you click "Create theme" a zip will be generated containing all your theme files. 5. You can install this like any other theme

This works without #4807 but it assumes we are taking that approach for Blockbase children, so that PR will need to be merged before this one is usable.

The logic is built on top of a REST API endpoint in core. I think eventually this should be part of core.

@scruffian scruffian requested review from alaczek and a team October 18, 2021 19:50
@scruffian scruffian self-assigned this Oct 18, 2021
*/

function gutenberg_edit_site_get_theme_json_for_export() {
$child_theme_json = json_decode( file_get_contents( get_stylesheet_directory() . '/theme.json' ), true );
Copy link
Member Author

Choose a reason for hiding this comment

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

In Blockbase this is currently a generated file, but once #4807 is merged this will make more sense.

}

function blockbase_get_theme_css( $theme ) {
return file_get_contents( get_stylesheet_directory() . '/assets/theme.css' );
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally this should output all enqueued CSS for the theme.

gutenberg_edit_site_export_theme( $_GET['theme'] );
}
}
add_action( 'init', 'blockbase_save_theme');
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 way too heavy handed, but it works for now.

@pbking
Copy link
Contributor

pbking commented Oct 18, 2021

This is brilliant! Per your comment this pairs really nice with the removal of child-theme.json and makes a nice, compact theme.json.

I haven't gone through the resulting theme carefully yet but as to the concept I believe it's a great looking tool.

@alaczek
Copy link
Contributor

alaczek commented Oct 19, 2021

This is awesome!

Would it be possible to create a child theme not from an existing child (Geologist > Zoologist), but directly from Blockbase? I think that would be the most useful in the context of the work I'm doing now.

Also, I expected was to get child-theme.json starter file and it wasn't there in the generated theme.

Lastly, I thought the generated theme would include package.json?

@pbking
Copy link
Contributor

pbking commented Oct 19, 2021

child-theme.json is ABOUT to go away, and that whole part of the build step too. Since there's also no scss in the generated theme there's also no css build step for these themes so the result is pretty simple.

I just tested building straight from Blockbase and it looks like it will need a few tweaks to do so efficiently (It creates a working theme, but with more stuff than needed). But no reason that wouldn't also work.

@MaggieCabrera
Copy link
Contributor

I love this! in fact, I like it so much that I think it should be the theme generator we were thinking about for https://blockbasetheme.com/

@scruffian
Copy link
Member Author

Would it be possible to create a child theme not from an existing child (Geologist > Zoologist), but directly from Blockbase? I think that would be the most useful in the context of the work I'm doing now.

Yes that should work fine.

Also, I expected was to get child-theme.json starter file and it wasn't there in the generated theme.

This is because we are getting rid of child-theme.json (see #4807)

Lastly, I thought the generated theme would include package.json?

Again, because we don't need the build step anymore we shouldn't need a package.json. I'm curious what @MaggieCabrera and @pbking think about this, since we might still want to build SASS in these child themes...

@MaggieCabrera
Copy link
Contributor

MaggieCabrera commented Oct 19, 2021

Again, because we don't need the build step anymore we shouldn't need a package.json. I'm curious what @MaggieCabrera and @pbking think about this, since we might still want to build SASS in these child themes...

I think it depends! Generally, I don't want to get rid of SASS for Blockbase at all for our own benefit. Thinks like the Buttons stylesheet using mixins and stuff would be very ugly to do using only CSS. But I agree that if the theme is based on Blockbase and it's only adding some theme.json changes, there's no point to do any kind of build process, and the extra CSS can be added without SASS. If you are basing on Quadrat, however, you are loading the extra import for the outlined buttons for example. That requires SASS unless we are refactoring Blockbase

@pbking
Copy link
Contributor

pbking commented Oct 19, 2021

Again, because we don't need the build step anymore we shouldn't need a package.json. I'm curious what @MaggieCabrera and @pbking think about this, since we might still want to build SASS in these child themes...

Don't build it 'till ya' need it.

If a generated theme advances such that it needs a sass build step then that's a decision for the future.

Ideally a theme based on Blockbase wouldn't HAVE any css either which is indeed the best scenario; when changes come down the pipe the child theme gets them without need of alteration whereas if it were build from something like Quadrat and pulls that theme's CSS in there may be reason for that CSS to change in the future based on changes to Gutenberg. Ideally most of that kind of stuff happens in Blockbase, that's the idea.

Definitely agree with Maggie though that we can't lose SCSS for Blockbase itself.

If you are basing on Quadrat, however, you are loading the extra import for the outlined buttons for example. That requires SASS unless we are refactoring Blockbase

I do think that if it's based on Quadrat then the Quadrat CSS is needed (for the types of reasons you state @MaggieCabrera ) , but I don't think it needs to have the SASS source come along for the ride. Though... if it were a child of QUADRAT instead then there's no need to include that CSS in the bundle either... Grandchildren Block Themes? 🤔

@alaczek
Copy link
Contributor

alaczek commented Oct 20, 2021

No build process sounds great to me! :)

This is because we are getting rid of child-theme.json

Oh ok, I wasn't aware of that. What does that mean to themes I'm currently developing from Blockbase - do I keep using child-theme.json?

@scruffian
Copy link
Member Author

Oh ok, I wasn't aware of that. What does that mean to themes I'm currently developing from Blockbase - do I keep using child-theme.json?

Yeah for now that's fine. We'll migrate all the themes over to the new process when the time comes, just make sure its in the list of child themes in rebuild-child-themes.js

@jeffikus
Copy link
Contributor

@scruffian was thinking about this "The logic is built on top of a REST API endpoint in core. I think eventually this should be part of core." - should we place it under the "Tools" menu item perhaps then? Or "Appearance"?

@MaggieCabrera
Copy link
Contributor

I'm thinking we may want to hide this for dotcom users if we ever merge this, and this kind of thing is probably not allowed on .org either as part of the theme

License URI: https://raw.githubusercontent.com/Automattic/themes/trunk/LICENSE
Template: blockbase
Text Domain: {$slug}
Tags:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we pre populate this with Blockbase's tags? We never really change them for child themes. At most you would had a "photography", "blogging", etc tag that would talk about the content, but most children will have the same functionalities as the parent

Copy link
Member Author

Choose a reason for hiding this comment

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

one issue is that our tags contain wpcom specific tags, which would be annoying for dotorg users.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, we can leave those for now and add them manually, but I think adding the blockbase ones is better than leaving it empty

@scruffian
Copy link
Member Author

I moved this to a plugin: https://github.com/Automattic/create-blockbase-theme

@scruffian scruffian closed this Oct 29, 2021
@scruffian scruffian deleted the add/create-theme branch October 29, 2021 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants