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

Git integration for themes #416

Closed
wants to merge 6 commits into from
Closed

Git integration for themes #416

wants to merge 6 commits into from

Conversation

madhusudhand
Copy link
Member

@madhusudhand madhusudhand commented Jul 14, 2023

🚧 WORK in PROGRESS 🚧

image

Related issues:

#435

@madhusudhand madhusudhand changed the title First iteration of the git integration for themes Git integration for themes Jul 14, 2023
@conatus
Copy link

conatus commented Jul 14, 2023

This is great!

@pbking
Copy link
Contributor

pbking commented Jul 17, 2023

It's an interesting idea, keeping the location of the git repository in the CBT plugin folder rather than directly in /themes. I'm keen to know your thoughts of why that is better than managing the resources directly in the /themes folder.

One potential downside is that I'm not able to connect my repository and then immediately have access to any theme that I might have just connected. That's also a positive too though; connecting the Automattic/themes repo doesn't immediately fill my theme selection with ALL of the themes.

Because of the folder structuring this seems best suited for a repository (like Automattic/themes) where the theme would be in a folder. Perhaps less well suited for a repository that has only one theme (like the many premium offerings, or most themes I see in the wild).

How do you imagine changing which branch a user is on would work?

Is this something better suited for integration in the editor rather than wp-admin?

The biggest and most important question though is of course "Is CBT the right place for this work?" I've gone back and forth on that question a dozen times over the past few months. It would be relatively easy to combine this work and the work I've done on Rivisr to bring the functionality right into CBT. Does that seem like it would have value?

@madhusudhand
Copy link
Member Author

madhusudhand commented Jul 18, 2023

Is this something better suited for integration in the editor rather than wp-admin?

Yes, Agree that it suites better within the editor. I did the first version in wp-admin because it is quick 😄

I'm keen to know your thoughts of why that is better than managing the resources directly in the /themes folder.

The main reasoning is that the theme cannot be committed directly to repository, because it requires transformations such as removal of ref, theme attributes. Having in a separate place, gives control to use the theme export logic and copy resources for commit without disturbing the original themes directory.
Also, thinking through themes with their own repositories (premium themes) as a use case, it is not possible to make themes directory as git.

this seems best suited for a repository (like Automattic/themes) where the theme would be in a folder. Perhaps less well suited for a repository that has only one theme (like the many premium offerings, or most themes I see in the wild).

I thought through this scenario and considered having this variable target_path.
This determines where the theme should go. When it is empty theme resources are directly committed in repo root (for premium themes). And when a path is given ex: pixl, resources would go under that path. (for Automattic/themes)
It should become a user input in the UI.

How do you imagine changing which branch a user is on would work?

This would be some interesting scenario to solve. Plugin could fetch the list of open branches from the repository using the access token or have a user input for the first iteration.

In this first iteration, I have always considered default branch HEAD as remote branch to create a new local branch. And the same local branch is pushed to remote after commit.
This could be the default behavior, and when user specifically wants to checkout a branch from the remote, we could capture it from the UI and then checkout.
We should probably show remote source branch, and local branch in the UI.

@pbking
Copy link
Contributor

pbking commented Jul 27, 2023

The main reasoning is that the theme cannot be committed directly to repository, because it requires transformations such as removal of ref, theme attributes. Having in a separate place, gives control to use the theme export logic and copy resources for commit without disturbing the original themes directory.

So far this is the biggest hurdle for me to get over.

This made sense at first glance, however I don't believe I agree. These transformations are necessary, but if they are necessary in the exported theme then are they not also necessary (or at least reasonable) in the installed theme? Themes 'saved' via CBT should be in a clean state. Theme attributes, ref attributes, etc shouldn't be in the templates; those should remain in the user space. If CBT isn't handling all of those scenarios well then that's a failing of CBT and not something I think we should try to work around.

Also, thinking through themes with their own repositories (premium themes) as a use case, it is not possible to make themes directory as git.

This also doesn't seem like a good reason to have the repository stored elsewhere. In the case of the Automattic public themes repository those themes would be in /wp-content/pub and /pub would be the managed folder. Sure that is ALL of the themes, but I don't see that as a problem. In the case of an Automattic premium theme, since they have their own repositories they would be in /wp-content/theme-slug and that would be the managed folder. That seems like the right avenue to me.

@pbking
Copy link
Contributor

pbking commented Jul 27, 2023

I tried to tinker in here yesterday and today and I really just haven't been able to get past the theme repository being outside of the /themes folder. I'm pretty convinced that this should be refactored to leverage the repository there instead of keeping it "somewhere else" and trying to work on it from there. I started to do that today but I got very confused and wasn't able to make much progress.

I'm going to put that down for now until we have an opportunity to discuss further.

@pbking
Copy link
Contributor

pbking commented Aug 3, 2023

Thank you for chatting with me about this. I do see the advantage of keeping the "working copy repo" outside of the /themes folder now. I'm still not convinced that the resources saved in the /themes folder shouldn't be "fully ready to go" and need further exporting. Regardless, managing the repos in this way makes for convenient possibilities.

@pbking
Copy link
Contributor

pbking commented Apr 29, 2024

git integration looks like it's not going to be a part of CBT (the closest being the playground integration).

Closing as "an interesting experiment".

@pbking pbking closed this Apr 29, 2024
@vcanales vcanales deleted the git-integration branch May 23, 2024 07:49
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.

3 participants