feat(#4033): add Vue wrappers#4047
Conversation
|
Preview links
Built from commit afc5fbd. Previews are removed automatically when this PR closes. |
bdfranck
force-pushed
the
benji/vue-wrappers
branch
5 times, most recently
from
cdbc5e7 to
d77b23d
Compare
|
@vanessatran-ddi @willcodeforcoffee We don't need to do an in-depth review yet. Just a quick "sanity check" to see if these Vue wrappers are largely in the right direction. |
Something is missing... 😉 |
bdfranck
force-pushed
the
benji/vue-wrappers
branch
from
d77b23d to
d832812
Compare
willcodeforcoffee
left a comment
willcodeforcoffee
left a comment
There was a problem hiding this comment.
This is some great progress @bdfranck ! I'm really impressed!
Like you said, this is early, but I like what you're doing so far! Looking forward to seeing the rest.
I wonder what @vanessatran-ddi thinks about the barrel files? I thought they added file clutter, but that is IMHO.
| @@ -0,0 +1 @@ | |||
| export { default as GoabAccordion } from "./accordion.vue"; | |||
There was a problem hiding this comment.
I don't think I like having per-component barrel files like this. I prefer one big file of exports at a higher level like we did with React and Angular.
There was a problem hiding this comment.
Oh yeah! We have single files for both React and Angular. I've changed it here as well.
| @@ -0,0 +1,24 @@ | |||
| <script setup lang="ts"> | |||
There was a problem hiding this comment.
Weirdly Card isn't working at all. I tried to find a solution quickly but came up dry.
It could be something to do with slot?
There was a problem hiding this comment.
Right, there are a bunch of "soft deprecated" components that we don't need to create Vue wrappers now. I've removed them because it will simplify testing and discourage future teams from using them.
| const customName = options?.renamedProps?.[key]; | ||
| const attrName = customName ?? transformFn(key); | ||
|
|
||
| if (key.startsWith("data-")) { |
There was a problem hiding this comment.
We do not need this data- logic, I checked on vue framework and they will auto set inheritAttrs true, https://vuejs.org/api/options-misc.html#inheritattrs
| } | ||
|
|
||
| export function useWcProps( | ||
| props: Record<string, unknown>, |
There was a problem hiding this comment.
Instead of flattenning props to string and unknown, we can do like this:
Reason is: if I make a mistake for example on tab.vue like this:
(disabled not disable), at least compiler will fail and tell me something is wrong:
| export const kebab: TransformFn = (input) => | ||
| input.replace(/([a-z])([A-Z])/g, "$1-$2").toLowerCase(); | ||
|
|
||
| export function transformProps<WC = Record<string, unknown>>( |
There was a problem hiding this comment.
We can remove this function as nowhere in vue-components called it
| </script> | ||
|
|
||
| <template> | ||
| <goa-dropdown v-bind="wcProps" :value="valueAttr" @_change="onChange"> |
There was a problem hiding this comment.
value is binding 2 times, one is wcProps and here.Should we exclude value from useWcProps
|
|
||
| <template> | ||
| <goa-popover v-bind="wcProps"> | ||
| <div v-if="slots.target" slot="target"> |
There was a problem hiding this comment.
in react and angular wrapper target slot is mandatory, vue we do not have a way to require for a slot, but I think we should add a warning message, perhaps add onMounted hook here and add warning if target slot is missing
3 participants
This PR adds Vue wrappers to our DS components. It also adds a Vue PR preview section with an example page of the wrappers in action.