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

Generic table component #818

Open
HigherOrderLogic opened this issue Oct 14, 2023 · 15 comments
Open

Generic table component #818

HigherOrderLogic opened this issue Oct 14, 2023 · 15 comments
Labels
enhancement New feature or request v3 #1289

Comments

@HigherOrderLogic
Copy link
Contributor

Description

Add generic to components such as UTable so that user can have better type control.

Additional context

UTable is a component that I use very often, and I usually find myself missing some properties of some rows. I think adding generics to suck components will help enhance the DX a lots.

@HigherOrderLogic HigherOrderLogic added the enhancement New feature or request label Oct 14, 2023
Copy link
Member

If I understand correctly, this would type the row when accessed via a slot?

<UTable :rows="items" :columns="columns">
  <template #quantity-data="{ row }">
    {{ row.quantity.value }}
  </template>
</UTable>

@bdebon
Copy link

bdebon commented Dec 7, 2023

I thinl you understand well @benjamincanac and I would also be super interested by that thing! 🥇

@HigherOrderLogic
Copy link
Contributor Author

If I understand correctly, this would type the row when accessed via a slot?

I meant when using the component directly, with support for generic through vuejs/rfcs#436.

Copy link
Member

What do you mean by using the component directly? Why would you care to type the rows if you're just using: <UTable :rows="rows" />?

@HigherOrderLogic
Copy link
Contributor Author

Why would you care to type the rows if you're just using: ?

Generic row will means that we can enforce type, eg: sort as { column: keyof RowT; direction: "asc" | "desc"; }, by as keyof RowT | (first: RowT, second: RowT) => boolean.

Copy link
Member

Honestly, I'm not really sure how to implement this if you have any clue!

@benjamincanac benjamincanac added the help wanted Extra attention is needed label Dec 8, 2023 — with Volta.net
@metkm
Copy link

metkm commented Dec 16, 2023

Honestly, I'm not really sure how to implement this if you have any clue!

What's the reason you didn't use the function signature or the script setup syntax for components? https://vuejs.org/api/general#function-signature

typing generic components is not possible (?) without using the function signature

@metkm
Copy link

metkm commented Dec 16, 2023

Why would you care to type the rows if you're just using: ?

Generic row will means that we can enforce type, eg: sort as { column: keyof RowT; direction: "asc" | "desc"; }, by as keyof RowT | (first: RowT, second: RowT) => boolean.

For now, you can do something like this

<UTable :rows="people">
  <template #name-data="{ row }: { row: typeof people[number] }">
    <p>{{ row }}</p>
  </template>
</UTable>

Copy link
Member

@metkm When open-sourcing this library with the whole App Config theming system, I had to rewrite everything with defineComponent instead of script setup to allow prop defaults: https://github.com/nuxt/ui/blob/dev/src/runtime/components/elements/Button.vue#L68

Copy link
Contributor

zoobzio commented Feb 23, 2024

Script setup syntax now supports default props: https://vuejs.org/api/sfc-script-setup.html#defineprops-defineemits

If we are OK w/ converting the component to use script setup syntax, I can create a proof-of-concept PR that implements generics.

Outside of the default props requirement, are there any other concerns that need to be kept in mind for such a conversion?

My only thought is that it seems desirable to use a consistent declaration syntax across all components defined in the library which could be an argument against such a change, but implementing generics could be a powerful feature for more components than just the table. Thoughts?

Copy link
Member

benjamincanac commented Feb 23, 2024

I guess we can consider this for v3. As we're gonna rewrite most of the components, this could be a nice opportunity to make the switch.

@benjamincanac benjamincanac added v3 #1289 help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Feb 23, 2024 — with Volta.net
Copy link
Member

benjamincanac commented Feb 24, 2024

@zoobzio Also, when using <script setup>, it won't be possible to use the config from mergeConfig to use in defineProps defaults as it will be a local variable: https://github.com/nuxt/ui/blob/v2.14.0/src/runtime/components/elements/Button.vue#L33

Example:
CleanShot 2024-02-24 at 17.51.04@2x.png

@metkm
Copy link

metkm commented Feb 24, 2024

@benjamincanac

Does it work with this syntax?

withDefaults(
  defineProps<{
    test: string
  }>(),
  {
    test: config
  }
)

Copy link
Member

Won't work either: https://vuejs.org/api/sfc-script-setup.html#defineprops-defineemits

The options passed to defineProps and defineEmits will be hoisted out of setup into module scope. Therefore, the options cannot reference local variables declared in setup scope. Doing so will result in a compile error. However, it can reference imported bindings since they are in the module scope as well.

Also not recommended to use type-based declaration for a module like this, it adds an extra compilation step to transform it to the same code you'd wrote using runtime declaration.

By the way there is an awesome video from @danielroe about this: https://www.youtube.com/watch?v=fA0ezJCGMuA

@zoobzio zoobzio mentioned this issue Mar 1, 2024
8 tasks
Copy link
Contributor

zoobzio commented Mar 1, 2024

I opened a PR that implements a working example of how generics might look in the library, just migrating to script setup and applying a generic type to the rows prop as a starting point.

I also played around w/ typing the column key like keyof Row but this creates type errors for implementations that have custom column keys like action in their tables so I avoided it for now.

To solve the problem of local vars not playing nice w/ defineProps, I inverted the order to map defaults over the props using defu, something like:

import { defu } from 'defu'
import { config } from '#ui/ui.config'
const props = defineProps({
  ui: {
    type: Object,
    default: undefined
})
const { ui } = defu(props, config.default)

@benjamincanac
Generics are powerful and I would be happy to help bring them into this library which I like a lot, the implementation in this PR is a very small use case and there are some improvements that I have in mind that might be really cool for v3 depending on the direction you want to take it. Take a look and let me know your thoughts!

@benjamincanac benjamincanac mentioned this issue Mar 3, 2024
8 tasks
@benjamincanac benjamincanac changed the title feat: generic components Generic table component Mar 5, 2024
@benjamincanac benjamincanac removed the help wanted Extra attention is needed label Mar 5, 2024 — with Volta.net
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v3 #1289
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants