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

Add Tailwind Support to WP-Scaffold #105

Open
1 task done
joesnellpdx opened this issue Apr 8, 2022 · 18 comments
Open
1 task done

Add Tailwind Support to WP-Scaffold #105

joesnellpdx opened this issue Apr 8, 2022 · 18 comments
Assignees
Labels

Comments

@joesnellpdx
Copy link
Contributor

joesnellpdx commented Apr 8, 2022

Is your enhancement related to a problem? Please describe.

Enhancement: Add Tailwind v3 support to WP-Scaffold.

A utility-first CSS framework packed with classes like flex, pt-4, text-center and rotate-90 that can be composed to build any design, directly in your markup.

Read more about Tailwind: https://tailwindcss.com/

Note: Opt-in functionality is limited to checking out the epic/tailwind-v3 branch or downloading a zip as described in the PR below

PR

#156

Contributors

@Firestorm980 @Nikki-Jones @dainemawer @iansvo

Designs

No designs required

Code of Conduct

  • I agree to follow this project's Code of Conduct
@joesnellpdx joesnellpdx changed the title Opt in tailwind for themes Opt in tailwind for theme Apr 8, 2022
@dainemawer
Copy link
Contributor

@Firestorm980 @Nikki-Jones - I would be keen to help out on this!

@iansvo
Copy link

iansvo commented Apr 20, 2022

I too would love to assist with this. I've had a crack at this recently at my past job and it would be interesting to revisit with some fresh perspectives.

I have a few thoughts that I've organized here below:

Classnames in blocks
One thing I would note though is there's no reason we can't just use utility classes in blocks that I can see. The fact that we have all of the block code in our theme directly makes it relatively trivial to leverage those classes as long as you're following the best practices for content purging (e.g. don't concat class names if at all possible, etc). This also then allows us to leverage the full benefits of JIT and arbitrary compilation which can really speed up the process.

Generator Command
One idea that might be beyond the scope of this but I still think could be sick if done correctly would be a sort of generator command that we can use, similar to 10updocker create if you will or create-react-app. Syntax like this:

npx @10up/scaffold-project --w=tailwindcss

I imagine the two-second solution would just be to checkout a given branch or something to that effect but this seems like it could also be useful beyond just this feature we're discussing, so I wanted to mention it.

@Firestorm980
Copy link

@iansvo I like the idea of having a generator command or similar. I think that makes more sense versus adding the files to the scaffold that would be required and having a hook or something like that. Like you said, it makes it more like create-react-app where you pick what parts you want and don't want, and I could see us extending that in the future with other options.

I have a few points I'd like discussion on as well. They don't need immediate answers, but we should give them some thought:

  • Do we limit or predefine the tailwind config in any way? (probably content directories for sure, but could include adding a prefix 10up-, for example)
  • Are there certain things we don't want people editing? (tailwind has loads of options, do we want it to be a free for all for any of them or a subset to start)
  • Do we try to link this up with theme.json in some way? (keeping parity with spacing, font sizing, etc.)

@dainemawer
Copy link
Contributor

The conversation going on here already sounds promising.
From my side, I think we need to determine a couple of things before we get into the architecture:

  1. Can we integrate tailwind into our build process easy enough (by extending webpack.config.js or postcss.config.js)
  2. Can we ensure that the above configs have a base layer of configuration that we'd consider to be the "default" and allow other engineers to customise template / component paths.
  3. Is it possible to get Tailwind to work with Frontity?
  4. One caveat of PurgeCSS which is built into Tailwind is that it does not evaluate dynamic classes: e.g article-post-<?php echo esc_attr( $variation ); ?> - how do we plan on tackling that?
  5. Will we need to set tailwind default global styles for some WP components that cannot be filtered or are output by core that we have no control over?

I think let's get all of our concerns down in this ticket. Regarding @iansvo generator idea, I like it! Though probably not through 10updocker - I'd say we should keep this in 10up-toolkit land https://github.com/10up/10up-toolkit mainly because of the following reasons:

  1. We may need to extend ESLint and Stylelint and maybe even Prettier?
  2. We're already in a Node-based environment, lets keep things neat and tidy and not over step our bounds in terms of Docker scripting haha.

It would be great to see this ticket owned by a number of us, so far we're on 4. Would a weekly or fortnightly sync be worthwhile? We can manage it async like the WP community does?

@Nikki-Jones
Copy link

@dainemawer @Firestorm980 @iansvo I love all the interest on this, get's me so excited!

Here's a little update on where we're at, sorry I didn't add this to the ticket initially. @Firestorm980 has a POC using an older version of tailwind. My goal is to update this to the latest version of the toolkit and tailwind to see how tailwind's new JIT compiler behaves with our setup. I can add this to the PR and then get some more of your feedback. The next steps I see after this would be:

  1. Abstracting it with the opt in functionality in the toolkit, I can hand this off for someone to pick up like @dainemawer's suggestion
  2. Best practice documentation for tailwind use in 10up's ecosystem - collaborative effort. Perhaps this would be a good point to create a tailwind working group and setup a couple of meetings

Some responses to your Q's:
@iansvo
Classnames in blocks - yes 100% agree, I don't see why we wouldn't leverage classnames in blocks. The newest update of tailwind has purging built in now, and it's quite easy to target certain directories for content: in the tailwind config.

@Firestorm980
Do we limit or predefine the tailwind config in any way? - I'm not sure what the benefit of limiting the config would be (aside from directories), considering only the utils added in your code will be output in the build with tailwinds new JIT compiler. I'm a little on the fence with prefixing, I get that it would be nice identifier of tailwind but I fret having to write out 10up- or tw- for every util.
Are there certain things we don't want people editing? - I think one of the main benefits of tailwind is all of the utils you get out of the box and they have added so many more in the past releases that it's rare I reach to override (other than colours and fonts). I would prefer to not limit the utils, and keep it open based on the brand/project needs and encourage anyone who uses tailwind to lean on their spacing increments especially. But great point, I think we should definitely roll out some best practice documentation for those using tailwind for the first time.
Do we try to link this up with theme.json in some way? - I would really love to add this, I have talked to @devinle and @joesnellpdx about this and the recommendation is to add this opt in as a starting point, and potentially incorporate the theme.json in a later release.

@dainemawer
Can we integrate tailwind into our build process easy enough (by extending webpack.config.js or postcss.config.js) - yes as far as we know with the @Firestorm980's POC, but still requires some investigation
Can we ensure that the above configs have a base layer of configuration that we'd consider to be the "default" and allow other engineers to customise template / component paths. - the latest release of tailwind provides a strong base by default, but no need to add this to the tailwind config.
Is it possible to get Tailwind to work with Frontity? - as far as I'm aware Frontity is no longer being maintained, is this something we would want to invest in?
One caveat of PurgeCSS which is built into Tailwind is that it does not evaluate dynamic classes: e.g article-post- - Tailwind has safelist options, we should definitely include this in the best practices for something to look out for
Will we need to set tailwind default global styles for some WP components that cannot be filtered or are output by core that we have no control over? - Yes this is something we will need to investigate, so far I have seen this done by targeting selectors with postcss-simple-vars

Let me know what you all think, I should be able to get this initial PR started by early next week :)

@dainemawer
Copy link
Contributor

dainemawer commented Apr 21, 2022

Thanks for the feedback here @Nikki-Jones ! Great stuff.

Okay so! Im going to add a checklist to this Github issue with some of our next steps. I think we should all be on the same page going forward before I do so, so Im putting this up for a vote, a simple 👍🏻 on this comment will do!

  1. @Nikki-Jones to update Jons POC to the latest version of Tailwind - let's add your findings to this ticket, also let's mention that PR in this ticket so that they are linked!
  2. We need to find someone who is willing to help us out with the 10up-toolkit scripting - does anyone want to own that?
  3. Setup a documentation template - this can be done either in 10up-toolkit or here in wp-scaffold - a simple README.md will do I think. Ideally, we just want to outline the benefits and how to get started initially, then we can hash the details out once we know how far we can get with this.

Does that sound good for next steps?

@joesnellpdx @devinle @Firestorm980 @iansvo @Nikki-Jones

@devinle
Copy link

devinle commented Apr 27, 2022

Hey, @Nikki-Jones and @dainemawer

Just wanted to drop my notes here in regards to Frontity. As far as I'm aware, we won't be using it moving forward on new projects. We'd likely move toward some sort of Next.js project.

I'm hyped to see this taking shape! I wonder if these tasks warrant living in TeamWork to distribute and document?

@dainemawer
Copy link
Contributor

Thats good to know, thanks for the update @devinle ! I don't think Teamwork is a bad idea at all! @devinle we may need your help with that, I cant create projects on Teamwork, don't have those super powers. Let me know if you can help us out there?

@dainemawer
Copy link
Contributor

@Nikki-Jones @devinle @Firestorm980 @iansvo here is a working doc that we can use to flesh this work out:

https://docs.google.com/document/d/1BEETids4ezBNmHJ64SgXwCcW8wLDRY-XaxfvjDunzKo/edit#heading=h.ayevlmr9vb1a

@iansvo
Copy link

iansvo commented Apr 29, 2022

@dainemawer Thanks for getting that started.

I put in some details just now for Goals, considerations, and next steps.

@devinle I personally would love to see this in a TW project once we get a few more things ironed out, should make it easier to keep track of.

@iansvo
Copy link

iansvo commented Apr 29, 2022

@Nikki-Jones @dainemawer @Firestorm980 I'm pleased to report I was able to get some resourcing time to work on this a bit today.

I've created a POC branch using v3, looked at @Firestorm980's POC as well during creation. Here's an overview of where everything's at with it:

https://github.com/iansvo/wp-scaffold/tree/feature/tailwind-v3

  • Initial integration in our build process complete
  • POC for ingesting theme.json values into the tailwind config is complete

I also added a few things into the doc that @dainemawer shared above, including some of what I'm about to mention.

One thing that's a huge roadblock here is Webpack's lack of support for glob patterns. This fundamentally complicates the process of watching for changes and using the content patterns (which in general use globs).

See the Engineering Considerations section in the doc for more details. Would love to get some thoughts on what should do to address.

@Nikki-Jones
Copy link

@iansvo @dainemawer @Firestorm980 here is the POC I was working on with more of a hybrid solution for bringing values from the theme.json in to the tailwind.config. You will notice I'm defining custom theme.json layout values and then extending the maxWidth values.

https://github.com/10up/wp-scaffold/tree/feature/tailwind-v3-hybrid

Some notable features:

  • The only plugin I brought in by default was for prose aka. @tailwindcss/typography
  • I'm styling the editor content by default with the prose classes in the components tailwind directive ie.@layer components
  • I have removed normalize in favour of tailwind's reset
  • I have transferred all existing out of the box css in the toolkit to tailwind

@iansvo I'm don't think I'm running in to the same webpack issues you're having. Could you check this out and let me know?

@iansvo
Copy link

iansvo commented Apr 29, 2022

@Nikki-Jones Yeah it looks like you weren't, thank god. It appears that I must've just not tested without the glob that catches root PHP files like I thought, or something. Plus the Tailwind docs indicate this actually shouldn't be able to work, so that added to it I guess.

I found that simply removing that one piece from the content globs actually solved my problem (derp). Either way, I'm glad the solution ended up being simpler than it appeared.

This is what I was testing with just now:

	content: [
		'assets/**/*.js',
		'includes/**/*.js',
		'includes/blocks/**/*.php',
		'includes/block-*/**/*.php',
	],

This is however somewhat of a problem because we need to be able to look at the default WP files. I feel like we could create a simple safelist pattern with relative ease though and just leverage that for this purpose, vs using a global blob.

My thought is that this was happening back of those asset.php files that we generate when we compile the files, which would be a **/*.php match for sure.

I"ll continue to mess around with a few things and provide some additional feedback/thoughts on your take. Super cool to see this taking shape!

@Firestorm980
Copy link

@iansvo @Nikki-Jones Looking at the branches, I think we could modify this config to handle most of the WP themes we deal with, and include some documentation on when / how to extend the config.

The following includes WP template hierarchy files for primary and secondary templates. If a theme needs a more specific template, it'd be up to the engineer to make sure they're included.

	content: [
		// Root files
		'404.php',
		'archive.php',
		'attachment.php',
		'author.php',
		'category.php',
		'date.php',
		'footer.php',
		'front-page.php',
		'functions.php',
		'header.php',
		'home.php',
		'index.php',
		'page.php',
		'search.php',
		'searchform.php',
		'single-post.php',
		'single.php',
		'singular.php',
		'tag.php',
		'taxonomy.php',

		// Directories
		'assets/**/*.js',
		'includes/**/*.{js, php}',
		'partials/**/*.php',
		'templates/**/*.php',
	],

Thoughts?

@iansvo
Copy link

iansvo commented Apr 29, 2022 via email

@Nikki-Jones
Copy link

@iansvo @Firestorm980 feel free to open a PR against the branch with the updated content list and any namespace patterns you come up with :)

@dainemawer
Copy link
Contributor

So great to see progress and commitment here 🚀

A few small pieces of feedback from my side to help us get aligned:

The only plugin I brought in by default was for prose aka. @tailwindcss/typography

@Nikki-Jones this is a great start! Any thoughts on adding https://github.com/tailwindlabs/tailwindcss-forms ? There are a lot of forms in WP (search, comments, etc)

The following includes WP template hierarchy files for primary and secondary templates

@Firestorm980 is it possible to make this somewhat extensible? We can carry on providing as many edge cases in here as possible, but the path of least resistance is to leave it up to an individual engineer at the end of the day.

Workflow

We kinda have 3 forks at the moment, Nikki's, Jon's and Ians - lets try to tame that so we can work in a more efficient manner. If the MVP of this project will exist as a branch (for now) - lets create an epic branch straight off of wp-scaffold, call it epic/tailwind and we can treat that as our trunk - any new features can be branched off of there and we can setup a PR against epic/tailwind - when it comes to 10up-toolkit we can determine a way forward there by using an unpublished version or something similar.

It would be great for us to choose which one of the 3 forks gets us as close as possible (considering our progress) to what we're trying to achieve, and work off of that stable codebase to figure our the rest of the priorities.

Based on my reading thus far, that appears to be @Nikki-Jones 's work? But I may be mistaken?

Safelisting
IMO, this poses the greatest threat. How do we reliably ensure that Tailwind does not strip out CSS from other plugins / enqueued CSS? As much as the answer may be obvious to some, we should at least ensure that we:

  1. Provide extensive documentation on this
  2. Test different scenarios
  3. Make it easy for other engineers to extend the safelisting configuration
  4. Put together a somewhat comprehensive list of classes that are generated by WP

VSCODE
There is an extension https://marketplace.visualstudio.com/items?itemName=bradlc.vscode-tailwindcss which we can include in this configuration using the .vscode folder which will suggest recommended plugins. It easy to setup.

Prettier
Any thoughts and opinions on including: https://tailwindcss.com/blog/automatic-class-sorting-with-prettier

@dainemawer dainemawer added this to the tailwind-beta-1.0.0 milestone May 10, 2022
@fabiankaegy fabiankaegy pinned this issue May 18, 2022
@fabiankaegy fabiankaegy changed the title Opt in tailwind for theme Add tailwind as an opt in feature to the 10up theme scaffold May 18, 2022
@dainemawer dainemawer mentioned this issue Aug 18, 2022
4 tasks
@dainemawer dainemawer added enhancement New feature or request help wanted Extra attention is needed Internal Review labels Aug 18, 2022
@dainemawer dainemawer changed the title Add tailwind as an opt in feature to the 10up theme scaffold Add Tailwind Support to WP-Scaffold Aug 18, 2022
@dainemawer
Copy link
Contributor

Pull Request: #156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants