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

Added a 'save' option #51

Merged
merged 11 commits into from
Mar 28, 2022
Merged

Added a 'save' option #51

merged 11 commits into from
Mar 28, 2022

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Mar 11, 2022

Added a 'Save Theme' option to save the theme.json and template changes to local theme folder and clear user changes.

Addresses: #45 and enables a development workflow.

image

To Test:

  • Start with a theme activated. (I have done most testing with Archeo but ANY theme [parent or child, Blockbase or not] should work fine.)
  • Make sure you begin with NO user changes to the theme (such as color, font or template changes)
  • Make changes to the theme using the FSE (or the Customizer). A good test for this is to change the color values of the theme colors.
  • Make changes to a template part (for instance add a simple paragraph block)
  • Make a change to a template (another simple change you can easily see)
  • Create a brand-new template and template part.
  • Use the Plugin to 'Overwrite'.
  • Load the site (view). Note that it still looks the same (global style changes and template changes are still in tact)
  • Attempt to "Reset to Defaults" the Global Styles using the FSE (nothing changes)

* Attempt to "Clear Customizations" from the template part you previously edited. Note that it is no longer an option.

* Observe the changes made to theme.json file in the theme in the filesystem. Note that the Global Styles changes made in the FSE are now represented in the theme.json file in `/wp-content/themes/SLUG/theme.json` * Note the changes made to templates in the theme filesystem, as well as additional templates/parts added based on the templates created with the FSE

This change also clears user settings when a theme is 'saved' addressing #50

@pbking pbking linked an issue Mar 11, 2022 that may be closed by this pull request
@pbking
Copy link
Contributor Author

pbking commented Mar 11, 2022

Thus far I've gotten this to SAVE the theme.json and templates.
It also REMOVES user Global Styles.
Currently trying to figure out how to delete user templates.

I have noticed that the json being exported is missing a few settings (such as appearanceTools:true and lineHeight:true. These things will be missing from any created theme.

@MaggieCabrera
Copy link
Contributor

Does that happen too with WordPress/gutenberg#39048 ? We should be using that function if we can and if it's not working correctly, patch upstream?

@pbking
Copy link
Contributor Author

pbking commented Mar 14, 2022

Does that happen too with WordPress/gutenberg#39048 ? We should be using that function if we can and if it's not working correctly, patch upstream?

The problem is also in Gutenberg's export. I'll be reporting that in detail directly.

This plugin does not yet use the logic to build theme.json from Gutenberg. Unfortunately that logic (added in #39048) isn't exposed in a way that we can take advantage of. But it is done in the same way as this plugin so the result is the same.

@MaggieCabrera
Copy link
Contributor

@scruffian to keep you in the loop

@pbking pbking requested a review from a team March 14, 2022 15:12
@scruffian
Copy link
Contributor

cc @draganescu

@pbking pbking linked an issue Mar 15, 2022 that may be closed by this pull request

//NOTE: Dashes are replaced with \u002d in the content that we get (noteably in things like css variables used in templates)
// This replaces that with dashes again.
$template->content = str_replace( '\u002d', '-', $template->content );
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like the original content was encoded, is it maybe this what's happening? Maybe we can use the decode function instead in case there's more than dashes that we should consider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, very good call. Thank you. That felt pretty shabby when I wrote it. :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried very hard to do this. :(
I can't figure out how.

AFAICT this is something that should be addressed in Gutenberg and until it is perhaps this "good enough" solution will be ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mentioned it to @scruffian on his latest PR to the export functionality in GB, hopefully there will be a fix for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this actually happens when the template is modified using the site editor...

@draganescu
Copy link

Just arrived here. Thanks for the ping @scruffian As I'll start working on WordPress/gutenberg#39194 this is a great inspiration (a.k.a. place to "steal" from haha).

My thinking on the core ticket was more to create one single REST endpoint that hanles this and to send from the editor only one command: update_theme_files, only when the theme was in "development" mode. I think if a theme is in development clicking update in the site editor should update the files themselves.

@pbking
Copy link
Contributor Author

pbking commented Mar 23, 2022

As I'll start working on WordPress/gutenberg#39194 this is a great inspiration (a.k.a. place to "steal" from haha).

That's the idea, please steal away. I just consider this a playground to determine what may be best to surface in Gutenberg. I'm testing out this functionality as much as I can for the development of Pendant.

I would love to lend a hand on #39194 @draganescu and will keep an eye on it. I didn't even actually know that issue existed until now. :)

@@ -414,6 +469,70 @@ function add_templates_to_zip( $zip, $export_type, $new_slug ) {
return $zip;
}

function add_templates_to_local( $export_type ) {
//NOTE: This was heavily copied from add_templates_to_zip
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I agree this needs a refactor ideally.

);
return $zip;
}

function add_theme_json_to_local ( $export_type ) {
$theme_json = MY_Theme_JSON_Resolver::export_theme_data( $export_type );
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this depend on another plugin? That's not ideal...

Copy link
Contributor Author

@pbking pbking Mar 23, 2022

Choose a reason for hiding this comment

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

It's not another plugin, just this that we had already added previously. All stuff that we had hoped would ultimately eventually be done by Gutenberg... maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this suffers from the same issues as the Gutenberg exporter, in that it removes $schema and appearanceTools keys. Feel free to use the code from that PR until it is resolved...

@scruffian
Copy link
Contributor

When I do a save I see this notice:

"New block theme created!"

We should update this to be more descriptive...

@scruffian
Copy link
Contributor

This converts tabs to spaces in theme.json. I think we should keep tabs for now.

@pbking
Copy link
Contributor Author

pbking commented Mar 24, 2022

This converts tabs to spaces in theme.json. I think we should keep tabs for now.

I agree, but how do you suggest we do that? I'm not sure that I'm keen for a code formatting to hold up the merge, but I would ultimately like to provide tabs, not spaces. Just not sure how to do that with PHP.

@scruffian
Copy link
Contributor

I agree, but how do you suggest we do that? I'm not sure that I'm keen for a code formatting to hold up the merge, but I would ultimately like to provide tabs, not spaces. Just not sure how to do that with PHP.

WordPress/gutenberg#39751!

@pbking
Copy link
Contributor Author

pbking commented Mar 25, 2022

Thanks for feedback, I have made some changes:

Message when saving:
image

theme.json is now saved with tabs, not spaces

The export code is refactored to be a lot more dry.

There is still an issue with $schema and appearanceTools, as does the Gutenberg exporting logic and I'm keen to let that settle before doing anything special here. I don't believe it's a blocker for now.

The unencoding is still hacked in, also looking for a better solution in Gutenberg and in the meantime the limited "decoding" that it's doing now I think is enough for what we're doing.

@scruffian
Copy link
Contributor

The unencoding is still hacked in, also looking for a better solution in Gutenberg and in the meantime the limited "decoding" that it's doing now I think is enough for what we're doing.

The resolution for this in Gutenberg was to do nothing - see WordPress/gutenberg#39576

Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM

I saw a couple of notices but I think they are unrelated to this PR:

[28-Mar-2022 11:26:13 UTC] PHP Notice: Undefined variable: old_slug in /var/www/html/wp-content/plugins/create-block-theme/admin/class-create-block-theme-admin.php on line 426
[28-Mar-2022 11:26:13 UTC] PHP Notice: Undefined variable: old_slug in /var/www/html/wp-content/plugins/create-block-theme/admin/class-create-block-theme-admin.php on line 439

@pbking
Copy link
Contributor Author

pbking commented Mar 28, 2022

I saw a couple of notices but I think they are unrelated to this PR:

They are caused by a recent change I did cleaning up the code. Thanks for catching. Patching that up.

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.

Consider Clearing User Changes Add "Save" option
4 participants