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

Tailwind v3 WP Scaffold #156

Closed
wants to merge 50 commits into from
Closed

Tailwind v3 WP Scaffold #156

wants to merge 50 commits into from

Conversation

dainemawer
Copy link
Contributor

@dainemawer dainemawer commented Aug 8, 2022

Tailwind Logo

Description of the Change

This PR adds Tailwind v3 support to wp-scaffold - the team has worked hard to set up the correct implementation, considering all avenues and pitfalls regarding WordPress and Tailwind working together seamlessly.

We have no intention of merging this PR into trunk - Tailwind should be an opt-in for projects that think they can benefit from its fast prototyping and well-thought-out utility CSS.

We have added plenty of documentation to this feature in order to ensure that getting up and running with the framework is as easy as possible:

Read the TAILWIND.md
Read the README.md

For more information and further help, you can reach out to the #10up-tailwind channel in Slack.

Issue

Closes #105

How to test the Change

  1. Download a zip of the epic/tailwind-v3 branch:
  2. Extract the files
  3. Replace your projects wp-content folder with the extract files
  4. Run npm install
  5. Tailwind purges unused CSS closes during the build process, so add any utility class to any .php, .js, file in the theme and run npm run build - see the templates/page-tailwind.php for a working example.

Changelog Entry

Added - Tailwind v3 Support

Credits

Props @Nikki-Jones @iansvo @Firestorm980 @dainemawer

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

Nikki-Jones and others added 30 commits April 29, 2022 09:46
Update content to target only explicit files at root with coverage for all default WP templates (thanks Jon!).

Added basic safelisting for bg and text primary, secondary, and tertiary colors.

Removed additional plugins I had added previously.
- Added WP breakpoint (600px) often used in core styles
- Container are now centered and have padding by default
- Removed additional colors that were already being defined by theme.json

Note: on the above for the colors, I’m just having the definitions come directly from theme.json, vs using the CSS variables (which are already CSS variables, or absolute values). I think its the same difference here since these are both coming from the same place anyway.
Added official plugins back to create a simpler DX and reduce friction during initial setup. Added a brief explanation for each and link to the plugin page in the Tailwind docs.
Setup separate file for tailwind helper JS
Added logic for opt-in importing for color palettes in blocks (includes dedup to prevent overwrites)

I tested this locally using a custom block style.
Colors are now retrieved directly from theme.json
Colors with the primary, secondary, and tertiary slugs are now safelisted via regex
CSS organization has been reorganized around the use of imports, vs one tailwind index file
Content definitions updated with list from Jon
Tested/confirmed pattern for ensuring WP class defs are not lost
Plugins are included by default
Add extensions.json in .vscode config
Full coverage for Admin bar and certain gutenberg styles
Tailwind: Additional config updates
Fix arbitrary class typos for reduced motion
Update placement of purgecssWordPress safelist.

Co-authored-by: Jon Christensen <[email protected]>
@dainemawer
Copy link
Contributor Author

Definitely will, thanks @iansvo !

@dsawardekar
Copy link

@dainemawer Super glad to see this initiative. I've dabbled with Tailwind a little and have been very impressed. The notes on where to use this version of the scaffold are great. In some ways this feels like the big transition from Classic Editor to Gutenberg. There will be growing pains but it feels worthwhile to invest the time.

As has already been discussed, this may not be suitable to merge into trunk and may end up being in a separate tailwind-trunk branch. I think that needs to revisited as maintaining 2 versions in the long run isn't great.

I have a proposal to make the WP Scaffold modular here, #104

With a modular scaffold, the Tailwind flavor of WP Scaffold could become a separate composer package. And the Theme author can choose to then declare support for Tailwind with a change like,

add_theme_support( 'tailwind' );

Great work!

@dainemawer
Copy link
Contributor Author

@dsawardekar - great to have you weigh in here!! It's a pretty big shift, I agree.
I think we're aligned regarding maintainability - it's not ideal to have it on a standalone branch, so some modular implementation or a CLI wizard I think would be our best bet in the long term.

I'll definitely chime in on your PR to add my support.

@Nikki-Jones @Firestorm980 @iansvo - any thoughts on Darshans proposal above?

@@ -0,0 +1,58 @@
const themeJSON = require('../theme.json');
Copy link

Choose a reason for hiding this comment

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

Overall here, I like this feature. Just curious of a few things.

  1. Could we add a header comment to this file, explaining its purpose?
  2. Could we add JSDoc commenting to each of the functions?
  3. Would this ever be problematic functionality if the theme.json had certain things removed by an engineer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@devinle - the above points sound good - @iansvo got any bandwidth to tackle this?

Copy link

Choose a reason for hiding this comment

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

Didn't see this until now but yeah I'll see about addressing a few things like this.

@devinle Can you elaborate a bit more on number 3 for me?

@Firestorm980
Copy link

@dsawardekar - great to have you weigh in here!! It's a pretty big shift, I agree. I think we're aligned regarding maintainability - it's not ideal to have it on a standalone branch, so some modular implementation or a CLI wizard I think would be our best bet in the long term.

I'll definitely chime in on your PR to add my support.

@Nikki-Jones @Firestorm980 @iansvo - any thoughts on Darshans proposal above?

The only thing I wonder about is integration with package.json and other dependencies. I'm sure that's already been figured out by someone, but it is a little different since we don't do modules like that currently. Sounds like a cleaner way to do it to me. It does make our work here hinge on that.

Copy link
Contributor

@joesnellpdx joesnellpdx left a comment

Choose a reason for hiding this comment

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

I honestly don't have time to test this, but I love all the thought and work that has been put into it!

  1. I agree with Devin's questions
  2. I love Darshan's suggestion and agree maintaining a separate branch will not only be difficult, but has a high chance of losing viability (from my experience)
  3. If we use this, are we therefore no longer going to leverage theme.json for establishing custom properties, global element styles, typography, color pallets, spacing, etc? Either way, there will need to be documentation on how to (or not to) integrate theme.json (apologies if that is already in place). Theme.json is becoming more and more powerful... so it will be difficult if we are to bypass it long term.
  4. Make it as easy as possible for "non tailwind" folks to pick up - or it will be a pain point
  5. Is there guidance for block development? Core blocks? Custom blocks? Are we intended to use @apply all over the place or are we using utility classes? If using @apply, how is that more efficient than a few lines of css? This will come up, so wanting to be sure this is thought through.
  6. Please be sure TL, Nicholas and Fabian get their eyes on it when you feel it is at final review stage.

@dainemawer
Copy link
Contributor Author

@dsawardekar - great to have you weigh in here!! It's a pretty big shift, I agree. I think we're aligned regarding maintainability - it's not ideal to have it on a standalone branch, so some modular implementation or a CLI wizard I think would be our best bet in the long term.
I'll definitely chime in on your PR to add my support.
@Nikki-Jones @Firestorm980 @iansvo - any thoughts on Darshans proposal above?

The only thing I wonder about is integration with package.json and other dependencies. I'm sure that's already been figured out by someone, but it is a little different since we don't do modules like that currently. Sounds like a cleaner way to do it to me. It does make our work here hinge on that.

@dsawardekar any thoughts on this?

@dainemawer
Copy link
Contributor Author

I honestly don't have time to test this, but I love all the thought and work that has been put into it!

  1. I agree with Devin's questions
  2. I love Darshan's suggestion and agree maintaining a separate branch will not only be difficult, but has a high chance of losing viability (from my experience)
  3. If we use this, are we therefore no longer going to leverage theme.json for establishing custom properties, global element styles, typography, color pallets, spacing, etc? Either way, there will need to be documentation on how to (or not to) integrate theme.json (apologies if that is already in place). Theme.json is becoming more and more powerful... so it will be difficult if we are to bypass it long term.
  4. Make it as easy as possible for "non tailwind" folks to pick up - or it will be a pain point
  5. Is there guidance for block development? Core blocks? Custom blocks? Are we intended to use @apply all over the place or are we using utility classes? If using @apply, how is that more efficient than a few lines of css? This will come up, so wanting to be sure this is thought through.
  6. Please be sure TL, Nicholas and Fabian get their eyes on it when you feel it is at final review stage.

@iansvo @Nikki-Jones @Firestorm980 - I think @joesnellpdx brings up some valid points here - how best can we tackle these in a meaningful way?

@joesnellpdx some thoughts from me based on your feedback:

  1. I like the points Devin brings up
  2. Extra work is required to either modularise this or build into some kind of CLI wizard / flag - Nicholas had some ideas on this previously I think.
  3. Hard to answer this - @iansvo @Firestorm980 @Nikki-Jones would love to get your feedback here
  4. Like any new adoption to technology, leveraging this framework requires some level of familiarity with Tailwind Paradigms, we've linked to documentation as much as we can - I think you're either in 2 camps: love Tailwind, hate Tailwind, if you love Tailwind you would have already tried your hand at it, if you hate Tailwind, you will pretend this feature doesn't exist haha.
  5. This point I do want to get clarity on - @Nikki-Jones @iansvo @Firestorm980 - either of you have a smart response for this?
  6. Roger that!

@Nikki-Jones
Copy link

@joesnellpdx great points, I added some comments in this PR to address 1,3,5: #162 thanks! cc: @dainemawer @iansvo

@dsawardekar
Copy link

@dainemawer Since we have 2 completely different dependency trees, I think we should keep this simple to start with. Once the things that are common are well defined we can look at dependency management tools like lerna.

I would keep a package-default.json and a separate package-tailwind.json. Then on bootstrap / checkout you have to,

$ cp package-tailwind.json package.json

This is easy enough to automate with a CLI / generator. We could also do the same on composer.json side if required.

@tobeycodes
Copy link
Member

tobeycodes commented Sep 9, 2022

What about having both themes in the theme folder? The readme can be clear that you need to choose one and delete the other.

If we want to add a CLI we could then copy Next.js with their create-next-app package. https://github.com/vercel/next.js/tree/canary/packages/create-next-app

yarn create wp-scaffold --tailwind - This would replace the tailwind theme with the default theme. In my opinion they should be maintained in separate folders and the cli should only be responsible for choosing which theme folder to use.

@dsawardekar
Copy link

Good point, I think there is merit to @tobeycodes's suggestion. If the 2 themes are independent and do not actually share any of the baseline styles / code, then having 2 separate starter themes would be pretty clean. It will also be nicer to maintain because the pull requests won't have any merge conflicts across themes.

@dainemawer
Copy link
Contributor Author

@dsawardekar @tobeycodes I really like the suggestion of doing something like: yarn create wp-scaffold --tailwind - how much lift are we looking at to get that implemented?

I'd like to outline some next steps here:

  1. Lets get this branch up to date with trunk again
  2. I'd like to do another round of testing
  3. Find a way to keep this branch updated and easily re-creatable by engineers using some form of CLI command

@dsawardekar
Copy link

dsawardekar commented Nov 28, 2022

@dainemawer Toby's suggestion above still feels like a cleanest way to proceed here. Instead of having these changes inside themes/10up-theme directory we could put them in themes/10up-tailwind-theme. That would avoid merge conflicts as the 10up-theme scaffold changes.

The scaffold generation feels like a separate feature that we should brainstorm first. Right now as noted in the docs we are recommending to do a search-replace for scaffold strings like tenup-theme. I haven't used yarn for a while, but there are tools like Yeoman that can help build custom project scaffolds. (Yeoman has been around for a while, there may be others scaffold generators that are more modern.)

@jacobdubailcadmv
Copy link

Resurrecting this thread to see if there are any clear action items that can be taken. I would like to rebuild some momentum on this initiative.

@fabiankaegy
Copy link
Member

Since we are not actively working on this at the moment I'm going to close the PR for now. The branch will still be there and the PR can always get revived

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 this pull request may close these issues.

Add Tailwind Support to WP-Scaffold
10 participants