Closed NullVoxPopuli closed 2 years ago
βοΈ Deploy Preview for ember-cli-guides ready!
π¨ Explore the source changes: 94a1d5cdbecb0b837aef7f52b1cd261837a6bb19
π 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
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.
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!
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.
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.
Spacing is a little weird around here: but I think that's out of scope for this PR?
@ember-learn/cli-core-team ready for rereview
I believe the spacing issue is a known bug, agreed that itβs out of scope.
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.
Thanks again for your work on this! And thanks for your patience. This slipped through my radar.
Sorry for commenting on a merged PR, but I hadn't seen this one before.
I noticed two things:
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--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 https://github.com/ember-learn/cli-guides/pull/256.
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
autoprefixer
was removed as a peer dependency (https://github.com/tailwindlabs/tailwindcss/pull/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.
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!
Resolves: https://github.com/ember-learn/cli-guides/issues/244