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

[Fix] Add Chakra UI integration #5031

Merged
merged 2 commits into from
Apr 8, 2022

Conversation

m3t
Copy link
Contributor

@m3t m3t commented Apr 4, 2022

I think @TimKolberger added a wrong argument for the installation of Chakra UI related packages in #3715
Whereas in the Chakra UI docs the packages live in dependencies devDependencies

Installation does not belong to devDependencies
Co-authored-by: Philzen <[email protected]>
@netlify
Copy link

netlify bot commented Apr 4, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit 5e36515
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62507dd09f62250009fa201e

@TimKolberger
Copy link
Contributor

Good catch 👍 thanks for updating!

@thedavidprice
Copy link
Contributor

Hi All! I think this one needs to be coordinated with the (herculean) effort happening in #4950

Looping in @Tobbe @nickpdemarco — could you two help plot a way forward here?

Thanks, everyone! Stoked to see Chakra getting a lot of attention.

@nickpdemarco
Copy link
Contributor

Aha! I thought something was up here... https://github.com/redwoodjs/redwood/pull/3715/files#r834573794

AFAIK this is totally fine to go in and won't disrupt #4950. It'll be a trivial merge and then I can treat Mantine the same way; installing to regular dependencies. @Tobbe might see something I don't, so I'll defer to him.

@thedavidprice
Copy link
Contributor

Ah, I just took a look at the code myself. This was actually an intentional choice, but I'm not sure it matters.

@jtoar we decided to install Tailwindcss in devDependencies. We followed that pattern with Chakra, but this is the second time there's been a PR with a suggestion to move to dependencies. It doesn't really matter because our build-step is in a development context (does it)? I just want to be consistent and feel like we should have a rationale either way.

@Tobbe
Copy link
Member

Tobbe commented Apr 7, 2022

99‰ of the time it won't matter if chakra/tw is a dep or devdep in a redwood app. The one time it does matter is if someone were to create an app/a library that uses RW that in turn was going to be installed as a dependency of some other app. In that case chakra and tw might not get installed even though it might be needed.

So I'd say go ahead and switch to installing them all as a proper dependency.

@jtoar
Copy link
Contributor

jtoar commented Apr 7, 2022

When I made the setup ui tailwindcss command, I took my cues from Tailwind CSS's docs1 which say to install it as a devDependency:

image

I just had a look at Chakra UI's docs and they recommend installing it as a dependency, so removing the -D option seems like the way to go.

@thedavidprice but I don't think we should remove the -D option on setup ui tailwindcss just to be consistent. Like @Tobbe said, I don't think it'd matter in the sense that nothing would break, but we would be including packages that—in Tailwind's case—don't need to be there. Ultimately these are different styling solutions. Tailwind is kind of like a compiler; you build your CSS before you deploy. You don't need Tailwind when you deploy the way you need Chakra since you're literally importing components from it.

I haven't used Chakra a ton yet and am not a Tailwind expert either so I could be misinformed! But right now I do feel like they're separate libraries and deserve separate setup commands.

Footnotes

  1. https://tailwindcss.com/docs/installation

Copy link
Contributor

This is really helpful and, I suggest, should establish our rationale going forward:

  • we do whatever the docs tell us to do

😂

Sound like a plan?

@jtoar
Copy link
Contributor

jtoar commented Apr 7, 2022

we do whatever the docs tell us to do

This is the way!

@thedavidprice
Copy link
Contributor

@jtoar thanks again!

@Tobbe @nickpdemarco FYI I'm merging this one now.

@m3t Thank you!

@thedavidprice thedavidprice added the release:chore This PR is a chore (means nothing for users) label Apr 8, 2022
@thedavidprice thedavidprice merged commit 1e48bf8 into redwoodjs:main Apr 8, 2022
@jtoar jtoar added this to the next-release milestone Apr 8, 2022
@Tobbe
Copy link
Member

Tobbe commented Apr 8, 2022

@thedavidprice Think you might have been a bit fast on the trigger here 🔫 🤠

image

Fix over here: #5085

Copy link
Contributor

Indeed. Grateful for friends like you watching my back!!

@m3t m3t deleted the fix-chakra-ui-integration branch April 11, 2022 16:58
@thedavidprice thedavidprice modified the milestones: next-release, v1.1.0 Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

6 participants