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 shadcn library #77

Merged
merged 13 commits into from
Jan 21, 2024
Merged

Add shadcn library #77

merged 13 commits into from
Jan 21, 2024

Conversation

dEdmishka
Copy link
Contributor

I've added brand new React components library which includes all necessary components, like accordion, form, button e.c.

@dEdmishka dEdmishka added the enhancement New feature or request label Jan 19, 2024
@dEdmishka dEdmishka self-assigned this Jan 19, 2024
Copy link

vercel bot commented Jan 19, 2024

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

Name Status Preview Comments Updated (UTC)
messenger-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 21, 2024 1:36pm

@DemianParkhomenko
Copy link
Member

I think it is better to add components one by one and only when you need them. Also, components should be already modified according to our design.

@dEdmishka
Copy link
Contributor Author

dEdmishka commented Jan 19, 2024

I think it is better to add components one by one and only when you need them. Also, components should be already modified according to our design.

Yeap, let's add components when they'll be needed:)

src/app/globals.css Outdated Show resolved Hide resolved
tailwind.config.ts Outdated Show resolved Hide resolved
@DemianParkhomenko
Copy link
Member

@dEdmishka Is it ready to review?

@dEdmishka
Copy link
Contributor Author

@dEdmishka Is it ready to review?

Em, it is.

@DemianParkhomenko
Copy link
Member

Now build is failed. Please, fix it. You can run npm run build locally to check it.

image

@dEdmishka
Copy link
Contributor Author

Now build is failed. Please, fix it. You can run npm run build locally to check it.

image

build-example
Locally everything works great. I don't know, why your build failde. Try run npm install, or something.

Copy link
Member

@DemianParkhomenko DemianParkhomenko left a comment

Choose a reason for hiding this comment

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

I think you should add a component from shadcn, to test how easily (difficult) you can use shadcn. Because now we cannot even check if our "theming" is right, and if does it fit for shadcn. Also, you can replace an existing component with a component from shadcn.

And ideally the diff should not be very big. So then it will be easier to maintain our components. You can run npx shadcn-ui diff to check this.

https://arc.net/l/quote/ntfpiwcp

@dEdmishka
Copy link
Contributor Author

I think you should add a component from shadcn, to test how easily (difficult) you can use shadcn. Because now we cannot even check if our "theming" is right, and if does it fit for shadcn. Also, you can replace an existing component with a component from shadcn.

And ideally the diff should not be very big. So then it will be easier to maintain our components. You can run npx shadcn-ui diff to check this.

https://arc.net/l/quote/ntfpiwcp

Did you find out why your build failed?

@DemianParkhomenko
Copy link
Member

DemianParkhomenko commented Jan 21, 2024

Now build is failed. Please, fix it. You can run npm run build locally to check it.

It fails in GitHub actions. Moreover it fails locally too.

image

I guess you renamed folder, but forgot to change tsconfig or something like that.

"@/ui/*": ["src/components/ui/*"]

@dEdmishka
Copy link
Contributor Author

dEdmishka commented Jan 21, 2024

Now build is failed. Please, fix it. You can run npm run build locally to check it.

It fails in GitHub actions. Moreover it fails locally too.

image

I guess you renamed folder, but forgot to change tsconfig or something like that.

"@/ui/*": ["src/components/ui/*"]

Yes, I changed the folder from incorrect 'UI' to 'ui', and lowercased some folders too, because some folders were lowercased, some not(I don't think that it were the right approach). So, I record these changes to config files too.
Should mention, that GitHub actions really bugged, and it could load changes so long. Also, I propose to remove Vercel build, 'cause no one could see the build on the Vercel site(except you, Demian)

Copy link
Contributor Author

@dEdmishka dEdmishka left a comment

Choose a reason for hiding this comment

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

That's it. Returned library configs, tested library button in FormAuth.tsx. And I again checked configs, everything fine and dandy.
@DemianParkhomenko @firehawk89 pls renew your reviews

Copy link
Contributor Author

@dEdmishka dEdmishka left a comment

Choose a reason for hiding this comment

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

I've removed bugged alias "@/ui", 'cause in VS Code is simply ignoring it.
Also rechecked all folder names, they should be and they are lowercased, in order to prevent future conflicts.

Copy link
Member

@firehawk89 firehawk89 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@DemianParkhomenko DemianParkhomenko left a comment

Choose a reason for hiding this comment

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

@dEdmishka Thanks for PR. Good job! Some minor comments to resolve and you can merge it.

src/components/ui/index/Button.tsx Show resolved Hide resolved
src/components/ui/index/Button.tsx Show resolved Hide resolved
@DemianParkhomenko
Copy link
Member

@dEdmishka @firehawk89 WDYT about renaming components files according to shadcn naming convention?

i.e. to kebab case ButtonThemeToggle.tsx -> button-theme-toggle.tsx

@dEdmishka dEdmishka merged commit 73e0d58 into main Jan 21, 2024
5 checks passed
@dEdmishka dEdmishka deleted the add-shadcn-library branch January 21, 2024 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants