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

Tailwind prefix #619

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from
Open

Tailwind prefix #619

wants to merge 19 commits into from

Conversation

zernonia
Copy link
Member

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

πŸ“Έ Screenshots (if appropriate)

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@zernonia zernonia linked an issue Jun 18, 2024 that may be closed by this pull request
2 tasks
@zernonia
Copy link
Member Author

@sadeghbarati there was some problem back then when I'm implementing the tw prefix, but I just had a look today and seems like the snapshot and everything looks good. Can you help review and check if it's all good? 😁

@sadeghbarati
Copy link
Collaborator

image

Just switch to your branch build the CLI and upload it to Stackblitz, seems not working yet, which is why I don't like test files πŸ˜†

https://stackblitz.com/edit/vitejs-vite-dzqk4g

@zernonia
Copy link
Member Author

Can you check again @sadeghbarati ? 😁

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jun 18, 2024

@zernonia fix: cater for cn function did fixed the cn prefix

I see prefix applied to all imports in all ui .ts files
https://stackblitz.com/edit/vitejs-vite-dzqk4g

image

@zernonia
Copy link
Member Author

Ahh nice catch @sadeghbarati ! Fixing!!!!

@sadeghbarati
Copy link
Collaborator

I think we should setup pkg.pr.new for shadcn-vue cli and module packages too,
Stackblitz upload limit will prevent me to test future PRs

@zernonia
Copy link
Member Author

Yeah we can do that.. that would really help to test out the PR 😁

Copy link

github-actions bot commented Jun 18, 2024

Deploying with Cloudflare Pages

Name Result
Last commit: b88fe615
Status: βœ… Deploy successful!
Preview URL: https://473d1d70.shadcn-vue.pages.dev
Branch Preview URL: https://refs-pull-619-merge.shadcn-vue.pages.dev

@zernonia zernonia marked this pull request as draft June 18, 2024 09:10
@zernonia
Copy link
Member Author

Yup not as easy as I thought hahaha.. usingStringLiteral is not powerful enough.. will convert to draft for now

@zernonia zernonia marked this pull request as ready for review June 18, 2024 14:24
@zernonia
Copy link
Member Author

zernonia commented Jun 18, 2024

@sadeghbarati I found this goldmind! https://vue-metamorph.dev/

It simplified the transformation soooooo much!!! I've refactor the transformer entirely to use vue-metamorph! Do help test it out ya!

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jun 18, 2024

I hit the upload limit 😭

I think vue-metamorph should be in dependencies

image


These tests are in TypeScript project

Button index.ts

All UI ts files import are still prefixed

  • CVA defaultVariants has issue with prefixes (this is not just in the button, the component that is using CVA is having this issue)
  • interface types/props also have the same issue
  • withDefaults also has the same issue

image

Button.vue

image


auto-form, chart-area|bar|donut|line

image

@sadeghbarati
Copy link
Collaborator

Now I understand why Codemod Team is having hard time working on Vue Codemods

@sadeghbarati
Copy link
Collaborator

sadeghbarati commented Jun 18, 2024

Tested JavaScript projects too, seems like detypes is not working well with vue-metamorph

Button index.js

image

@zernonia
Copy link
Member Author

zernonia commented Jun 19, 2024

@sadeghbarati Do you mind checking again? This time the transformation for prefix is much stricter to paritcular attributes and function only.

Also made a patch for vue-eslint-parser that vue-metamorph is using underneath. The undefined issue should be gone now

@sadeghbarati
Copy link
Collaborator

@zernonia Everything is fine just

  • /* METAMORPH_START */ comment is in the some of components after parsing

  • sonner toast-options classes in not having the prefixes (ignore defineProps error I just add the components for test)

    image

  • v-calendar style section is not having the prefix which we could ignore duo to legacy package and new calendar implementation

@sadeghbarati
Copy link
Collaborator

in JavaScript projects, found a strange thing, happening in AccordionTrigger and SelectItem and maybe other components as well

image

cn placement is not formatted and also an additional ] is added to the end of the cn function

@sadeghbarati
Copy link
Collaborator

in JavaScript projects, found a strange thing, happening in AccordionTrigger and SelectItem and maybe other components as well

image

cn placement is not formatted and also an additional ] is added to the end of the cn function

Looks like this is detypes bug not vue-metamorph

Related Discussion #632

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.

[Feature]: Add tailwind prefix option to components.json
2 participants