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

Feature/19138441 tailwind v3 testing #160

Merged
merged 7 commits into from
Aug 25, 2022

Conversation

Nikki-Jones
Copy link

Description of the Change

After using Tailwind with 10up Toolkit on a real project there were some changes made to the base configuration. These modification help Wordpress Gutenberg styling from conflicting with Tailwind's reset. The developer experience is also improved with the addition of Stylelint and nesting.

Changelog Entry

Added - Stylelint and stylelint config
Changed - Postcss config
Changed - Tailwind preflight reset to custom reset
Changed - style.css import structure

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 Nikki-Jones changed the base branch from bug/19141384-tailwind-v3-pr-fixes to epic/tailwind-v3 August 16, 2022 22:31
Copy link
Contributor

@dainemawer dainemawer left a comment

Choose a reason for hiding this comment

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

@Nikki-Jones some great additions here, really stoked! 👍🏻 from me!

themes/10up-theme/.stylelintrc Show resolved Hide resolved
themes/10up-theme/assets/css/frontend/tailwind/index.css Outdated Show resolved Hide resolved
themes/10up-theme/assets/css/frontend/style.css Outdated Show resolved Hide resolved
Copy link

@iansvo iansvo left a comment

Choose a reason for hiding this comment

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

@Nikki-Jones I'm glad you were able to get this put together and there's some great stuff in here to help tighten up the DX/ergonomics.

I have 1-2 things that I would like to get the team's take on before we merge this and make sure we're all on the same page, and I believe there's perhaps a few small cleanup items as well.

I tried to leave some detailed feedback just let me know if you (or anyone else) want to talk through anything.

themes/10up-theme/.stylelintrc Show resolved Hide resolved
themes/10up-theme/TAILWIND.md Show resolved Hide resolved
themes/10up-theme/assets/css/frontend/base/wordpress.css Outdated Show resolved Hide resolved
themes/10up-theme/assets/css/frontend/tailwind/reset.css Outdated Show resolved Hide resolved
themes/10up-theme/postcss.config.js Outdated Show resolved Hide resolved
themes/10up-theme/tailwind.config.js Show resolved Hide resolved
themes/10up-theme/tailwind.config.js Outdated Show resolved Hide resolved
themes/10up-theme/assets/css/frontend/tailwind/index.css Outdated Show resolved Hide resolved
@dainemawer dainemawer added this to the tailwind-beta-1.0.0 milestone Aug 18, 2022
Copy link

@iansvo iansvo left a comment

Choose a reason for hiding this comment

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

Lookin pretty nice! I just had one point of discussion I wanted to get the group's take on but otherwise I'm with it!

I really love the attention to detail on this CSS reset (the default normalize is...not great even in the best of scenarios).

Really nice work here.

Comment on lines +14 to +16
@layer components {

}

Choose a reason for hiding this comment

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

@iansvo @Nikki-Jones I don't know if we've tried this so let me know if I'm crazy, have we combined @layer and @import? I realize why we'd want a layer, but I'm just thinking about if a engineer wants a lot of their own components.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @Firestorm980, could you add an example?

Choose a reason for hiding this comment

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

@Nikki-Jones So, for example, let's say I have 10+ custom components. Would I do:

@layer components {
  @import url('components/card');
  @import url('components/select');
  /* ... a bunch of other imports */
}

Or, something else? (It occurs to me you may @import and then use @layer in the component itself.)

And to be clear, I'm not saying something is wrong with this code, more an implementation question that you may or may not have the answer to.

@iansvo iansvo merged commit 53cb4d3 into epic/tailwind-v3 Aug 25, 2022
@iansvo iansvo deleted the feature/19138441-tailwind-v3-testing branch August 25, 2022 16:36
This pull request was closed.
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.

4 participants