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 appendix #245

Merged
merged 13 commits into from Apr 27, 2022
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

Resolves: #244

@netlify
Copy link

netlify bot commented Jan 6, 2022

✔️ Deploy Preview for ember-cli-guides ready!

🔨 Explore the source changes: 94a1d5c

🔍 Inspect the deploy log: https://app.netlify.com/sites/ember-cli-guides/deploys/61e055d6e99adb0007d53275

😎 Browse the preview: https://deploy-preview-245--ember-cli-guides.netlify.app

@jenweber
Copy link
Contributor

jenweber commented Jan 6, 2022

Very nice! I have a few questions and suggestions.

How much of these steps are specific to tailwind vs other libraries? It would be nice to have if we taught the overall strategy and then gave tailwind as the example. This is just an idea to take or leave.

Change request: per our usual conventions for URLs for Ember resources, the URL should be a generic concept, and we maintain URLs forever. Libraries come and go in popularity, so this helps us to make sure that we can adapt library-specific content without killing URLs.

The URL is separate from what is shown in the sidebar. We also usually try to name the sidebar topics in a generic way too, but in this case I think the word tailwind needs to be in there for discoverability.

guides/appendix/tailwind.md Outdated Show resolved Hide resolved
guides/appendix/tailwind.md Outdated Show resolved Hide resolved
guides/appendix/tailwind.md Outdated Show resolved Hide resolved
guides/appendix/tailwind.md Outdated Show resolved Hide resolved
guides/appendix/tailwind.md Outdated Show resolved Hide resolved
guides/appendix/tailwind.md Outdated Show resolved Hide resolved
@NullVoxPopuli
Copy link
Contributor Author

How much of these steps are specific to tailwind vs other libraries? It would be nice to have if we taught the overall strategy and then gave tailwind as the example. This is just an idea to take or leave.

I kinda like this. The gist is that you can integrate any cli/watcher/builder utility with ember without ember-specific hooks. With the knowledge of what ember-cli watches and how assets can be accessed, anything is possible. I'm not sure how to best phrase that and use tailwind as an example though. :-\

Change request: per our usual conventions for URLs for Ember resources, the URL should be a generic concept, and we maintain URLs forever. Libraries come and go in popularity, so this helps us to make sure that we can adapt library-specific content without killing URLs.

makes sense! will change!

NullVoxPopuli and others added 6 commits January 6, 2022 20:43
Co-authored-by: Jen Weber <weberj10@gmail.com>
Co-authored-by: Jen Weber <weberj10@gmail.com>
Co-authored-by: Jen Weber <weberj10@gmail.com>
Co-authored-by: Jen Weber <weberj10@gmail.com>
@jenweber
Copy link
Contributor

jenweber commented Jan 8, 2022

My next steps as a reviewer are to check in with the rest of the learning team before we press "go." We do this for all new pages. We have our next meeting Thursday. I think you have addressed most of the things I could imagine as concerns.

I left a couple more comments with some wording to help imply that Tailwind is not an "Ember" thing but part of the broad JS ecosystem. I also want to make sure we make it clear that most of the time, you install a dependency and it just works, but some things need a little more finesse. From my perspective, this can be shipped after those additions are included, or something similar to them.

@jenweber
Copy link
Contributor

jenweber commented Jan 8, 2022

P.S. thank you so much for writing this up! A common theme we hear from users is that it's too hard to find info like this - it's lost in Discord chats. The more we can share, the better, IMO.

@NullVoxPopuli
Copy link
Contributor Author

Spacing is a little weird around here:
image
but I think that's out of scope for this PR?

@NullVoxPopuli
Copy link
Contributor Author

@ember-learn/cli-core-team ready for rereview

@jenweber
Copy link
Contributor

I believe the spacing issue is a known bug, agreed that it’s out of scope.

@jenweber
Copy link
Contributor

If you want it to be different, I think you can add one more line of text below the bulleted list. I don’t think an extra return alone is enough.

@jenweber jenweber merged commit b68bff6 into ember-learn:main Apr 27, 2022
@jenweber
Copy link
Contributor

Thanks again for your work on this! And thanks for your patience. This slipped through my radar.

@bertdeblock
Copy link
Collaborator

bertdeblock commented Apr 28, 2022

Sorry for commenting on a merged PR, but I hadn't seen this one before.

I noticed two things:

  • Since this guide uses Tailwind CLI, both autoprefixer and postcss (including the config file) are not needed and should be removed, Tailwind CLI does autoprefixing itself and PostCSS is only needed if you include Tailwind as a PostCSS plugin
  • Tailwind CLI also supports a --minify flag, which we should probably include since the build script creates a production build by default?

We should probably update this once Embroider is included by default?
I think Embroider + PostCSS + Tailwind as a plugin is a slightly nicer experience and bundling will also be taken care of by webpack.

Thanks for doing this! This has helped me as well with Ember + Tailwind JIT! 👍

EDIT: Made #256.

@NullVoxPopuli
Copy link
Contributor Author

both autoprefixer and postcss (including the config file) are not needed

pnpm tells you to install them. They may be peer deps, but they aren't marked as optional. This totally could have changed since this PR was put up, but based what i'm seeing, tho warnings would still be present.

script creates a production build by default?

Aren't all assets minified by ember tho ?

Minifying already minified code gets slow

@bertdeblock
Copy link
Collaborator

autoprefixer was removed as a peer dependency (tailwindlabs/tailwindcss#7949) and postcss seems to be a normal dependency at the moment, so I would not expect any warnings or errors.
Also, the official "Tailwind CLI" getting started guide doesn't mention to install these: https://tailwindcss.com/docs/installation.
The PostCSS config file is only needed if you actually have a PostCSS setup.

Aren't all assets minified by ember tho?

No, I don't think Ember does any optimization for assets in ./public.
I've tested this, and the --minify flag is needed to have an optimized tailwind.css file.

@NullVoxPopuli
Copy link
Contributor Author

official "Tailwind CLI" getting started guide doesn't mention to install these:

Oh i know, but i don't trust docs.

I've tested this, and the --minify flag is needed to have an optimized tailwind.css file.

Excellent! Thanks!

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.

Using TailwindCSS with Ember
4 participants