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

feat(cli): A bit smarter cli #823

Closed
wants to merge 11 commits into from
Closed

Conversation

Bekacru
Copy link
Contributor

@Bekacru Bekacru commented Jul 3, 2023

Hey @shadcn and @dan5py I was seeing issues like these https://github.com/shadcn/ui/issues/818 appear repeatedly. The issue, I believe, stems from using constant initial values on the cli, so I figured it would make sense to offer better initial values based on project info. I'm not sure whether you guys are working on it with more features, but since it's not a difficult task, I thought I'd give it a shot.

@vercel
Copy link

vercel bot commented Jul 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 3, 2023 4:29pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-template ⬜️ Ignored (Inspect) Visit Preview Aug 3, 2023 4:29pm

@vercel
Copy link

vercel bot commented Jul 3, 2023

@Bekacru is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@dan5py
Copy link
Contributor

dan5py commented Jul 3, 2023

Hi @Bekacru. You're right, those kind of issues are more and more frequent. I need to test your solution but it looks promising. Thank you 💪.

@dan5py
Copy link
Contributor

dan5py commented Jul 3, 2023

Just tested it out and it works very well. Just to complete your work could you please run this command:

pnpm format:write

Because when running the check it complains for the formatting. Thanks again, good job!

@Bekacru
Copy link
Contributor Author

Bekacru commented Jul 3, 2023

done

Copy link
Contributor

@dan5py dan5py left a comment

Choose a reason for hiding this comment

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

@shadcn 👍

@shadcn
Copy link
Collaborator

shadcn commented Jul 4, 2023

@Bekacru Awesome work! Now that we have docs for several frameworks, I wonder if we can auto-detect framework and provide better defaults for the CLI?

@shadcn shadcn added enhancement New feature or request area: cli labels Jul 4, 2023
@Bekacru
Copy link
Contributor Author

Bekacru commented Jul 4, 2023

@shadcn thanks! yeah that's possible. I think most of them use similar naming like app, pages, and components, as well as the folder alias is the same so we only need to tweak them slightly for each of them. Do u want me to work on that?

@shadcn
Copy link
Collaborator

shadcn commented Jul 4, 2023

Do u want me to work on that?

If you got time, yes please!

@dan5py
Copy link
Contributor

dan5py commented Jul 4, 2023

I should also have time these days. I’ll see what I can do. 💪

@Bekacru
Copy link
Contributor Author

Bekacru commented Jul 4, 2023

Do u want me to work on that?

If you got time, yes please!

on it 👍

@Bekacru
Copy link
Contributor Author

Bekacru commented Jul 5, 2023

@shadcn and @dan5py started working on this. since all of them follow almost similar folder structure, the present projectInfo is sufficient for the most part, but I added a styles folder resolver to better identify the global CSS. Let me know if there is anything I missed.

Copy link
Contributor

@dan5py dan5py left a comment

Choose a reason for hiding this comment

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

Thank you. I suggest just few changes.

packages/cli/src/utils/get-project-info.ts Outdated Show resolved Hide resolved
packages/cli/src/utils/get-project-info.ts Show resolved Hide resolved
packages/cli/src/utils/get-initial-values.ts Show resolved Hide resolved
packages/cli/src/commands/init.ts Outdated Show resolved Hide resolved
packages/cli/src/utils/get-initial-values.ts Show resolved Hide resolved
@shadcn
Copy link
Collaborator

shadcn commented Jul 12, 2023

@Bekacru quick update here. This is looking good. I'll review and get this merged this week.

@shadcn shadcn self-assigned this Jul 13, 2023
@shadcn
Copy link
Collaborator

shadcn commented Jul 14, 2023

I started testing this and it looks like we need to put in some work. We need to update the helpers so that config is read from cwd eg. getTsConfig is reading the local tsconfig.json and not from target dir.

@Bekacru
Copy link
Contributor Author

Bekacru commented Jul 14, 2023

I started testing this and it looks like we need to put in some work. We need to update the helpers so that config is read from cwd eg. getTsConfig is reading the local tsconfig.json and not from target dir.

I'm not sure if this is what you're looking for, but adde cwd to all helper path resolvers in the same way as you did for getConfig.

@shadcn shadcn added the area: roadmap This looks great. We'll add it to the roadmap, review and merge. label Oct 21, 2023
@shadcn shadcn added this to the next milestone Oct 21, 2023
@shadcn
Copy link
Collaborator

shadcn commented Jan 28, 2024

@Bekacru We shipped automatic detection in the cli. We're doing this on a framework-basis. I'm going to close this issue as done. Feel free to reopen if you have any questions. Thanks for your work on this PR. Appreciate it.

@shadcn shadcn closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cli area: roadmap This looks great. We'll add it to the roadmap, review and merge. 🚀 autorelease enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants