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

Enhance/site editor to SAVE user changes, EDIT metadata, EXPORT as a zip, CLONE to create #368

Merged
merged 20 commits into from
May 24, 2023

Conversation

pbking
Copy link
Contributor

@pbking pbking commented May 12, 2023

While working with Revisr (the git integration tool I'm working on for the Theme Development Pipeline) I ran into a few things that I was unable to do from the Editor (requiring that I use CBT's wp-admin page) or that I was unable to do at all (requiring direct file manipulation).

The tooling to do these things didn't fit well into Revisr and @matiasbenedetto had already started a branch showing examples of how some of these changes might work. So I have integrated some of those things into the editor and I'm introducing them in this change request.

  1. Top-level Menu
image

The first change moves the 'export' functionality currently at the "top level" into a page and moves it behind the "Export Zip..." menu item. This allowed me to introduce other top-level menu items giving users the option to Save Changes and Modify Metadata.

  1. Export instead of Clone
image

The current "export" functionality is actually a "clone" of the theme where MetaData and namespacing can be changed. That is handy functionality, but that seems better suited to a "create" submenu item. (It was my intention to introduce that in this change but it was the most complicated change and this was already growing into a pretty big change.) So rather than allowing a user to input any MetaData in this page they are instead offered JUST an "export" button that exports the theme as a .zip file. It's nearly the same kind of functionality as the native 'export' but it does the "CBT Magic" that native Gutenberg export doesn't.

  1. Edit Metadata
image

This new menu item allows a user to change the metadata (stored in style.css) from the editor. It doesn't allow the user to change the theme NAME (since that would require changing a lot more than a value in style.css and is more akin to CLONING the theme than just modifying it).

  1. Move Theme Location

One of the items in the "Edit" screen is now 'Theme Subfolder'. This was one of the primary things that I needed to add in order to work with themes with Revisr. When a theme is installed from a .zip file it's installed at the top-level of wp-content/themes. However to manage themes in a git repo (such as our /pub free themes) that theme needs to be in a subfolder. This new functionality lets a user move the theme from a top-level into a subfolder (or move it around into another subfolder, etc). This works my simply moving the folder and then re-activating the theme with the new location information

  1. Save Changes
image

Making changes to templates and Global Styles only changes the values in the user space. This was fine when the workflow is to export a (cloned theme) .zip. However in order to get those changes into the local file-system (for management with Revisr) those changes have to be saved. This works well in the wp-admin portion of the plugin since the goal with this workflow is to keep all of the functionality in the Site Editor this was added as well. In order for the "export" functionality (as it exists in this branch) to export the user changes they must be 'Saved' first.

NOTE: It may also be helpful to give the user the option to SAVE THE CHANGES TO A VARIATION (though I believe that native variation management might be better than this option)

  1. API refactoring

This change introduces a new PHP class that handles all of the API logic. This moves it out of the class-create-theme.php and gives us a good place to cleanup that logic. I think that ultimately we can sunset the wp-admin pages and thus the class-create-theme.php logic in lieu of this new class.

  1. Create Themes
image

On this last submenu page a user is able to CREATE a new theme in three different ways.

First they are prompted for the same metadata they are presented with in the EDIT submenu page. Once entered that information is used to create a new theme via:

CLONE: Copy the current theme and all of the user changes to a NEW theme with the entered metadata. Functions and templates will have the namespacing changed to the new theme slug. This will be created on the server and activated.

EXPORT: This is nearly the same as CLONE except the theme is exported as a ZIP file instead of saved to the server.

CREATE BLANK THEME: A blank theme with no templates or styles is created with the provided metadata. It is created on the server and activated.

@pbking pbking changed the title Enhance/site editor to SAVE user changes, EDIT metadata and EXPORT (not clone) as a zip Enhance/site editor to SAVE user changes, EDIT metadata, EXPORT as a zip, CLONE to create May 16, 2023
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, these are some great additions and it sounds like a lot of this work will unblock the work with Revisr ✨

Export instead of clone

It's nearly the same kind of functionality as the native 'export' but it does the "CBT Magic" that native Gutenberg export doesn't.

Allowing the user to only export the theme in this option makes sense to me 👍

Edit Metadata

Nice! I think it would be helpful to add a tooltip/description text to some of the items, like the small text we currently use on the wp-admin page:

image

Something like this:

image

But we could include this in a follow-up issue.

Save Changes

In order for the "export" functionality (as it exists in this branch) to export the user changes they must be 'Saved' first.

Maybe we should also include an option to 'Save & Export' on the Export Zip sidebar? We could keep the 'Export' only option as well, so users can still just do a simple export.

API refactoring

I love how you've done this, and this looks like it sets up the plugin well to move away from the wp-admin pages.

Create Themes

This is a follow-up task but I noticed with the Create New and Edit Info options that the user can't edit the theme tags along with the other metadata. I know there isn't much space available in the sidebar, but maybe the tags input could behave more like the Categories or Tags in the Post Editor. That way they wouldn't take up as much space. They could also be collapsed by default.

image

I noticed the screenshot has yet to be added too, but I saw the TODO items and I realise both the tags and the screenshot aren't currently included in this sidebar anyway.

I've left a few inline comments as well about issues I noticed while testing this out, but maybe they're specific to my environment. I'm using WP 6.2.1 and the latest GB trunk.

.then( () => {
createInfoNotice(
__(
'Theme created successfully. The editor will now reload.',
Copy link
Member

Choose a reason for hiding this comment

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

Small note, but I think this notice reads as if the editor will reload automatically, I didn't immediately realise that I needed to dismiss the notice to trigger it.

Maybe it could say something like: 'Theme created successfully. Dismiss this notification to reload the editor.'

I don't usually dismiss notifications, so maybe this is specific to me 😅

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 would actually prefer to make this notification a blocking one so that there's no option but to dismiss it. I looked for a native Gutenberg way to do that but I didn't find anything. Alternately I was also thinking about a good old-fashioned alert.

Copy link
Member

Choose a reason for hiding this comment

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

A blocking notification would be better here. Is there maybe a native blocking modal that could be used?

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 to refactor this to use 'alert()`, however the linter complained about using that. I wasn't able to find anything similar native to gutenberg UI tooling though.

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 just switched them to alert() calls and told eslint to ignore it. Think that's OK?

Copy link
Member

Choose a reason for hiding this comment

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

Just tested this out. I think that works well! We can always revisit it if we create/find a suitable Gutenberg UI. Thanks for doing that!

src/editor-sidebar/create-panel.js Outdated Show resolved Hide resolved
Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

@mikachan was already very thorough; I only want to add a minor nitpick.

This is excellent work!

Comment on lines +34 to +37
$old_slug = wp_get_theme()->get( 'TextDomain' );
$new_slug_underscore = str_replace( '-', '_', $new_slug );
$old_slug_underscore = str_replace( '-', '_', $old_slug );
$old_name = wp_get_theme()->get( 'Name' );
Copy link
Member

Choose a reason for hiding this comment

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

Every call to wp_get_theme creates a new instance; I think it might be worth keeping just one and reusing it.

Suggested change
$old_slug = wp_get_theme()->get( 'TextDomain' );
$new_slug_underscore = str_replace( '-', '_', $new_slug );
$old_slug_underscore = str_replace( '-', '_', $old_slug );
$old_name = wp_get_theme()->get( 'Name' );
$theme = wp_get_theme();
$old_slug = $theme->get( 'TextDomain' );
$new_slug_underscore = str_replace( '-', '_', $new_slug );
$old_slug_underscore = str_replace( '-', '_', $old_slug );
$old_name = $theme->get( 'Name' );

Copy link
Member

Choose a reason for hiding this comment

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

Going a step further, declaring a static $theme on the class could also save some of these calls.

private static $theme = wp_get_theme();

and then use it like

self::$theme;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you; good call.

That method was used throughout the app prior to this change and it would be good to clean that up in all the places. I hesitate to do so outside of the bits changed here so instead I've opened a new issue regarding cleaning up those calls throughout that perhaps we can address in the future.

Copy link
Contributor

@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.

This is working well, most of my comments are about clarifying how the actions are reorganized and presented / labeled. Nice work!

src/plugin-sidebar.js Outdated Show resolved Hide resolved
src/plugin-sidebar.js Outdated Show resolved Hide resolved
src/plugin-sidebar.js Outdated Show resolved Hide resolved
src/plugin-sidebar.js Outdated Show resolved Hide resolved
</Text>
<Spacer />
<Button variant="secondary" onClick={ handleExportClick }>
{ __( 'Export Theme', 'create-block-theme' ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is export exposed here, if it also has it's own panel? I assume it's because you can edit the theme metadata here, but if that's the case, should metadata editing be exposed in the export panel too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is different than "export the theme" and was one of the options I had the most trouble with.

THIS export option allows you to change the metadata of what is being exported. The changed metadata exists ONLY in what is exported. It's not persisted and exists only in the .zip.

The OTHER export option exports only what you have saved on the site. You can technically achieve the same thing by:

  • Changing Global Styles (storing it to user space)
  • Changing Templates (storing it in user space)
  • Saving the Theme (moving the changes from user space to theme space)
  • Editing the Metadata (saving it to theme space)
  • Exporting the theme

Wheras with this option you can skip "Saving the Theme" and "Editing the Metadata" and just create a brand-new theme exporting it.

This might be a userful tool for somebody who is using the plugin but is unable to write to the themes folder. They can make user changes but can't save them to the theme. Their only option would be to export the zip in this way.

This is more like "create" (since it's creating a brand new theme just like "clone" but exporting it as a zip) than it is "export" which is just exporting what is saved as a .zip

src/editor-sidebar/create-panel.js Show resolved Hide resolved
src/editor-sidebar/create-panel.js Outdated Show resolved Hide resolved
@pbking
Copy link
Contributor Author

pbking commented May 22, 2023

Based on @jffng 's feedback above I made the following changes:

  • Moved EXPORT and SAVE to top-level actions
  • Changed the ellipsis to chevronRight icon
  • Added explication to all top-level items
  • Changed icons
image
  • Changed the sub-menu titles
image image
  • Changed location and design of "create" items
image

Copy link
Contributor

@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.

Nice design improvements I think, I have a couple comments that can either be addressed here or in follow ups, but giving the ✅.

Some unexpected behavior:

  • I changed the theme subfolder (it was themes before, I deleted it, and saved, to test the behavior)
  • I used the export feature from the main panel
  • The zip is downloaded as zip.zip

return (
<PanelBody>
<Heading>
<NavigatorToParentButton icon={ chevronLeft }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now click on the heading text is also the back button:

Screenshot 2023-05-22 at 2 13 14 PM

I think only the chevron should be the back button, if that's possible.

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

I like the new alert() functionality and the design improvements are looking great. I think we can bring this in and iterate on it in further PRs if needed. Good stuff! 🚢

@pbking
Copy link
Contributor Author

pbking commented May 23, 2023

Some unexpected behavior:
...The zip is downloaded as zip.zip

I have discerned that this is because the "update metadata" portion of updating the theme is removing things from style.css that it shouldn't be. That includes the text_domain value which is what the zip filename is based on.

Making a quick fix to that before bringing this in.

…n' metadata fields on edit. Added 'tags' to edit.
@pbking
Copy link
Contributor Author

pbking commented May 23, 2023

image

Theme URI was getting wiped. I fixed that.
Version wasn't editable, now it is.
Tags are now editable (just a text string with ,'s) too
The textDomain field was getting wiped on edit, that was causing the zip.zip filename. That's fixed.

I think this is good to bring in and ship now!

@pbking pbking merged commit 1cfd8b1 into trunk May 24, 2023
1 check passed
@pbking pbking deleted the enhance/site-editor-update-metadata branch May 24, 2023 12:54
@pbking pbking mentioned this pull request Jul 31, 2023
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.

None yet

4 participants