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: prevent override default boolean prop value with useForwardProps #610

Conversation

sadeghbarati
Copy link
Contributor

@sadeghbarati sadeghbarati commented Jan 14, 2024

Related PR in shadcn-vue

radix-vue/shadcn-vue#272

image


PopperContent.vue already has avoidCollisions: true prop by default, in other Primitive Wrappers (SelectContentImpl.vue)
We pass down the same PopperContent types props but Vue will cast boolean props as false which will override the Main Primitive (PopperContent.vue) prop value

Now I fully understand why you create useForwardProps


I also have to check this PR too

radix-vue/shadcn-vue#241

Which I mostly pass down props instead of forwarded props

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Jan 14, 2024

vuejs/vue#4792

You were there too 🥲

Maybe we should tag "Evan You" just like useId issue?

@zernonia
Copy link
Collaborator

Thanks @sadeghbarati ! I read somewhere mentioning removing props casting would cause alot of breaking change, thus they held back from doing so atm. Since we have a work around for now, I think we can stick with it.

When a better time comes, we can bring it up again and perhaps have another macro to handle this 😁

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Jan 15, 2024

Done!


perhaps have another macro to handle this

IMO it's better to have fewer APIs in Vue, I don't want them to introduce new APIs or Macros, We want Stable API

@sadeghbarati
Copy link
Contributor Author

What about Combobox (Command) in radix-vue docs, Combobox is ok in shadcn-vue

It has inline position by default, why not popper

RadixVue Combobox

image

MeltUI Combobox

image

@sadeghbarati
Copy link
Contributor Author

sadeghbarati commented Jan 15, 2024

Linking related issue so boolean casting gets more attention by Vue team, hopefully

vuejs/core#8576
vue-macros/vue-macros#396

@zernonia zernonia merged commit cd33af8 into radix-vue:main Jan 16, 2024
2 checks passed
@sadeghbarati sadeghbarati deleted the transfer-boolean-props-to-undefined-with-useForwardProps branch January 16, 2024 16:41
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.

None yet

2 participants