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

Themes - Remove site-specific attributes from block markup #82047

Closed
miksansegundo opened this issue Sep 22, 2023 · 9 comments
Closed

Themes - Remove site-specific attributes from block markup #82047

miksansegundo opened this issue Sep 22, 2023 · 9 comments

Comments

@miksansegundo
Copy link
Contributor

miksansegundo commented Sep 22, 2023

Related WordPress/create-block-theme#162 Automattic/themes#6855

What

Remove the following site-specific attributes:

Why

These attributes break themes because the values of these attributes are IDs that exist only on the site used as a development environment when developing a theme.

See p1694095271631269-slack-C029FM1EH and p1693893212165259-slack-C029FM1EH

How

  • Use regular expressions to remove them from the theme files when exporting using CBT plugin
    • Use importers/exporters model. Not sure what this means, ask Sarah Norris. See p1693933333867029/1693893212.165259-slack-C029FM1EH
    • Try removing the attributes from the templates, parts and patterns content in the function add_templates_to_local
    • This solution won't work if the theme files are edited manually before are pushed to the repository
      • I think it's common for Themers to test changes in the editor, then copy the markup code from there and paste it into the theme files they have in their local environment before pushing the code to the repo.
  • Use pre-commit lint rules in Themes repo.
    • Return an error prompting Themers to remove the attributes with a script or run the script directly?
    • Also in wpcom-pub-themes, wpcom-a8c-themes, and wpcom-premium-themes repos?
    • See wp-scripts which uses eslint internally
    • See lint-staged which is used in Themes repo.
    • See deploy scripts in theme-utils.mjs
      • Maybe run a script before every deploy?
    • See pre-commit-hook.js
  • Maybe add a script in theme-batch-utils.sh to remove the attributes in batch in the whole repo?
@miksansegundo
Copy link
Contributor Author

Hi @Automattic/t-rex and @Automattic/lego, what are your thoughts on how to accomplish this task?

We know how easily these attributes can break blocks and mess up templates after theme activation or break theme previews on the onboarding and theme showcase.

@fushar
Copy link
Contributor

fushar commented Sep 29, 2023

@miksansegundo Maybe I'm missing something but isn't there already works on it via WordPress/wordpress-develop#3899?


This solution won't work if the theme files are edited manually before are pushed to the repository
I think it's common for Themers to test changes in the editor, then copy the markup code from there and paste it into the theme files they have in their local environment before pushing the code to the repo.

Ah really? I thought the "canonical" way is to always export the final theme files from the Editor.

@miksansegundo
Copy link
Contributor Author

Maybe I'm missing something but isn't there already works on it via WordPress/wordpress-develop#3899?

@fushar, have a look at these threads:

  • p1694096388620039/1693847600.113029-slack-C048CUFRGFQ
  • p1693928798971659/1693893212.165259-slack-C029FM1EH

@miksansegundo
Copy link
Contributor Author

I asked about the Themers workflow after a theme is exported in p1695973345286369/1694095271.631269-slack-C029FM1EH

@fushar
Copy link
Contributor

fushar commented Sep 29, 2023

@fushar, have a look at these threads:

Sorry, I missed the Slack links that are already mentioned in the issue description 🤦

Okay, I get the full picture now. I like the idea of a pre-commit hook. Maybe a stronger solution is to set up a GitHub action that blocks a PR when it still has site-specific attributes. (I think a pre-commit hook can still be bypassed manually by devs; it's not 100% safe.)

Regardless, do you have already any plan on how to detect the attributes? I'm assuming we want to do some regex trickery here. 😄

@mikachan
Copy link
Member

👋 Thanks so much for looking into this ahead of any of these issues being addressed in Core. This will be a massive help to our Themers.

Remove the following site-specific attributes:

This list looks good and covers all the site-specific attributes listed here, which is my best reference for this at the moment.

Try removing the attributes from the templates, parts and patterns content in the function add_templates_to_local

This sounds like a good plan, either using the add_templates_to_local function or a new function that is called when a theme is exported (probably via the export functions export_theme and export_child_theme).

Use pre-commit lint rules in Themes repo.

I'm a fan of pre-commit hooks and I'd suggest adding rules via lint-staged as you've mentioned above. However, @fushar is right in that pre-commit hooks can be bypassed manually, so this isn't always going to be reliable.

See deploy scripts in theme-utils.mjs

I'm also in favour of this idea, either as well as or instead of the pre-commit lint rules. We also check things like if a GlotPress project has been created for a theme pre-deployment, so I think a remove-attributes script would fit in well around that workflow.

@dsas
Copy link
Contributor

dsas commented Oct 16, 2023

what are your thoughts on how to accomplish this task?

My thoughts (and I have no specific knowledge of these issues) is that CBT/GB should handle this as far as possible because that's where the files come from, and what the community will use.

If the attributes can be reintroduced from elsewhere then it'd also be helpful if we had some git scripts to detect these issues and prevent them, and where possible a script to fix them once they are found.

@autumnfjeld autumnfjeld added [Pri] Low Address when resources are available. and removed [Pri] Low Address when resources are available. labels Oct 24, 2023
@taipeicoder
Copy link
Contributor

Also tracked in WordPress/create-block-theme#162

@miksansegundo
Copy link
Contributor Author

It's already on Pixel's team board marked as a high priority.

@miksansegundo miksansegundo closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants