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

Use shadcn components #154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mayur1377
Copy link
Contributor

Description

add shad-cn components

Type of Change

  • 🛠 Improvement to an existing snippet

note : there are some linting errors from the already pre-built shad-cn components , would recommend to disable folder that folder

  { ignores: ["node_modules", "dist", "build", "src/component/ui"] },

Copy link

netlify bot commented Jan 3, 2025

Deploy Preview for quicksnip ready!

Name Link
🔨 Latest commit ea49c91
🔍 Latest deploy log https://app.netlify.com/sites/quicksnip/deploys/677808e6dd9f3b00083b31e8
😎 Deploy Preview https://deploy-preview-154--quicksnip.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mayur1377 mayur1377 changed the title Add shadcn Use shadcn components Jan 3, 2025
@psychlone77
Copy link
Collaborator

Instead of ignoring the components/ui folder, we could run npm run lint -- --fix to fix those issues. Since those files are also technically part of the codebase which could be modified in the future, better to have those files also kept properly.

@mayur1377
Copy link
Contributor Author

Instead of ignoring the components/ui folder, we could run npm run lint -- --fix to fix those issues. Since those files are also technically part of the codebase which could be modified in the future, better to have those files also kept properly.

agree , but it would literally just be an overkill , to keep running it each and every time to fix the linting errors for the already pre built components
i would recommend two option here ,
-> ignore the components/ui folder which has all the pre-built shad-cn components which might hardly require some changes

-> just add the npm run lint fix command to the workflow file

Screenshot 2025-01-03 at 9 17 36 PM

@Mathys-Gasnier
Copy link
Collaborator

I'm too thinking we should ignore the components from shad-cn, it will just be a pain to reformat them each time.

@psychlone77
Copy link
Collaborator

agree , but it would literally just be an overkill , to keep running it each and every time to fix the linting errors for the already pre built components.
i would recommend two option here ,
-> ignore the components/ui folder which has all the pre-built shad-cn components which might hardly require some changes
-> just add the npm run lint fix command to the workflow file

We will only have to fix the linting errors once when someone adds a new 'shadcn component' to the project. Since 'shadcn' components is still someone else' s code and we are technically just copy pasting the components into the project, we should still make sure that code follows the project's eslint rules.

Running npm run lint -- --fix is an option, but might introduce changes which the person who made the commit might not have expected. It's better for the build to fail and show where error was, instead of it automatically making changes.

@mayur1377
Copy link
Contributor Author

mayur1377 commented Jan 7, 2025

any updates of still using shadcn?

@Mathys-Gasnier
Copy link
Collaborator

Fix conflicts and errors of your PR before we review again

@jjcantu
Copy link
Contributor

jjcantu commented Jan 10, 2025

@Mathys-Gasnier I think we should probably open up another MR for this. It is blocking a lot of other developers from contributing.

@psychlone77 psychlone77 added future Might implement in the future update needed Code needs to be updated. labels Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future Might implement in the future update needed Code needs to be updated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants