-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use Tailwind #3
Use Tailwind #3
Conversation
68dc172
to
dc91a7f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - it was easy for me to install tailwind following the instructions and your setup all seems sensible to me. I've left a couple of suggestions on typos., and a slightly more interesting one to potentially make running the app with tailwind a bit simpler, but nice work 😄
README.md
Outdated
``` | ||
npx tailwindcss -i ./todo_app/tailwind.css -o ./todo_app/static/css/index.css --watch | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not important, just in case interesting
I often feel that having a separate extra command for this kind of thing is slightly annoying as a dev; this definitely isn't important but if we can bake it into the "default" run command, I always think that's nicer. I have used poetry's scripts
section before, but it looks like it's not great at running arbitrary commands and certainly not combos (e.g. this discussion) - one option might be this plugin but given that you've brought in NPM, I know we can solve this there!
For example, we could:
- Install the
concurrently
package, so we can run two commands in parallel with a single command - Add a scripts section to our
package.json
that looks something like:
"scripts": {
"css": "npx tailwindcss -i ./todo_app/tailwind.css -o ./todo_app/static/css/index.css --watch",
"dev": "concurrently \"npm run css\" \"poetry run flask run\""
}
And then we can start both css and app together with npm run dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooo thats very nice, I also didn't like having to run extra commands but hadn't considered enforcing it on run... but that makes a lot of sense as you would always want the current css 😄
Co-authored-by: JackMead <jack.mead@corndel.com>
Co-authored-by: JackMead <jack.mead@corndel.com>
Make sure to follow the instructions in the README... happy to remove the generated css file from the gitignore but it seems to be standard practice not to include it as it would add bulk to reviews that isn't neccisary.