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 TailwindCSS colors (and script to update them) #25

Merged
merged 14 commits into from
Jun 15, 2023

Conversation

jmuchovej
Copy link
Contributor

👋 Following up on #24, I've added the colors from TailwindCSS (an incredibly popular CSS framework).

Their code is MIT licensed and their colors don't appear to have a different license, so it seems that these colors are also MIT licensed.

I've also run the tests using julia --project=. tests/runtests.jl and received the following output:

Test Summary: | Pass  Total  Time
tailwind      |    2      2  0.1s
Test Summary: | Pass  Total  Time
xkcd          |    2      2  0.0s
Test Summary: | Pass  Total  Time
Resene        |    2      2  0.1s
Test Summary: | Pass  Total  Time
NBS           |    2      2  0.0s
Test Summary: | Pass  Total  Time
X11           |    2      2  0.0s
Test Summary: | Pass  Total  Time
CSS3          |    2      2  0.0s
Test Summary: | Pass  Total  Time
Crayola       |    2      2  0.0s
Test Summary: | Pass  Total  Time
WinsorNewton  |    2      2  0.0s
Test Summary: | Pass  Total  Time
named lookup  |    4      4  0.1s
Test Summary:                        | Pass  Total  Time
colorant_str macro gets named colors |    4      4  0.0s
Test Summary:                                  | Pass  Total  Time
colorant_str macro gets original functionality |    2      2  0.0s

Adds JavaScript script to track the colors that ship with the `tailwindcss`
`npm` package. Also edits `.gitignore` to prevent tracking `node_modules`
and `package-lock.json`
Copy link
Collaborator

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me; I noticed that the sources in the Readme are not up to date (even missing my last Paul Tol colours update ehem), could you at least add your source there as well?
(Feel free to list https://personal.sron.nl/~pault/ as well, since I forgot that).

test/loading.jl Show resolved Hide resolved
scripts/exportTailwindColors.cjs Outdated Show resolved Hide resolved
@jmuchovej
Copy link
Contributor Author

jmuchovej commented Jun 5, 2023

So, I've added the link to Tailwind's (and Paul Tol's) colors in the README. I also removed the copy-paste error in test/loading.jl. Hold-off on accepting any changes 'cause Tailwind changed the colors and I forgot to verify that the tests pass.

I was able to extract most of the JS code into a Julia script – so if that's preferable, let me know and I'll remove the JS code and opt for the Julia script.

(Otherwise, I can trim the Julia code back down to the ~10 line script it was and let JS do its thing.)

To get the Julia script running, I needed to add JSON and NodeJS, but I'm not sure how Julia packaging works with "dev dependencies" (i.e., JSON and NodeJS aren't needed to use NamedColors, so I didn't wanna muddy the Project.toml if unnecessary).

@kellertuer
Copy link
Collaborator

If the update script is in its own folder, it could get its own Project toml for example? Just within the scripts/ folder do Pkg.activate(".") (or ] activate . and add the packages there. If you want to be very precise add even a [compat] section to that project toml as well.

Sure it should not be additional dependencies for NamedColors.

But the rest already looks very good :)

@jmuchovej
Copy link
Contributor Author

Done! Should I leave the JS code for reference, or take it out entirely? (I left it as reference mostly in case I ever needed to go back, but I could also make it a gist and just link to it from the update-tailwind.jl file?)

If you're good with leaving the JS code as reference, then this is ready to merge. 🎉

@kellertuer
Copy link
Collaborator

If the Julia script now does the Job well, we could remove the old JS code to avoid confusion? So maybe a gist and linking it might be indeed the less confusing resolution here.

Besides that – I agree, we are nearly done – thanks for reacting to all my ideas and comments.

Removes the JS script used to fetch Tailwind colors from their JS package and adds a link to a private GitHub Gist in the source of `scripts/update-tailwind.jl`.
@jmuchovej
Copy link
Contributor Author

Removed! 🙂 I linked the Gist in the header of scripts/update-tailwind.jl. At worst, someone would have to have to dig through the git history to find the original JS code (if they needed it). ¯_(ツ)_/¯

Copy link
Collaborator

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

🚀
I will wait a bit if some other developers have something to comment, but to me it looks good and I would merge it maybe end of the week?

We could also set the patch version one up (in the main project.toml) to register this as a new patch version.

@kellertuer
Copy link
Collaborator

Could you maybe as a last step increase the patch number in the project toml? Them I would merge it and register a new version :)

@kellertuer
Copy link
Collaborator

Thanks 👍

@kellertuer kellertuer merged commit c5b4068 into JuliaGraphics:master Jun 15, 2023
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.

2 participants