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

feat(Table): generic table #1446

Closed
wants to merge 4 commits into from
Closed

feat(Table): generic table #1446

wants to merge 4 commits into from

Conversation

zoobzio
Copy link
Contributor

@zoobzio zoobzio commented Mar 1, 2024

πŸ”— Linked issue

Resolves #818

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ 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

This PR converts the table component to use script setup syntax to take advantage of generic types. The refactor involved some changes to properties formerly passed to defineComponent that now needed to be defined w/ utility functions (defineOptions / useAttrs / etc).

The biggest change involved how prop defaults are handled due to the inability to reference local vars in the declaration. The PR inverts the current process by applying the defaults after props are defined rather than before, w/ the props defaulting to undefined which is automatically ignored by defu therefore the mapped local vars will inherit values from the defaults.

Mapping the results to local vars should mean that the template will use those mapped values rather than the one in props. However, this caused conflict w/ some computed props that were slightly renamed and then modified to use the local vars but still checking props to see if the user explicitly sets as null.

The only other thing to note is that the column key could optionally by typed as keyof Row rather than string, but this will create type conflicts for any user that adds custom column keys like action in the examples so it was avoided. We could consider adding a string | keyof Row type but the benefits of this are negligible unless we introduce a mechanism that allows the user to explicitly extend the available keys in the type.

The library is designed to support components that use the defineComponent syntax so this additional feels out of place in contrast w/ the other components but it could serve as a POC for a broader implementation of generics that I think could improve the DX and might be worth building into v3.

πŸ“ Checklist

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

shinGangan and others added 4 commits February 23, 2024 17:40
…77f1 (#1405)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
- convert component to use `script setup` syntax to prepare for generics
  support
- prop definition can't use local vars w/ `script setup`, set defaults to undefined & use
  `defu` to merge defaults where applicable
- props mapped as local vars should overwrite direct prop references in
  the template
- modify `loadingState` & `emptyState` computed properties, if the
  corresponding props are explicitly set as `null` they will pass `null`
  to maintain existing functionality
- cast `rows` prop to a new generic type that extends a basic object
  representation
- TODO: consider using `keyof Row` type on column keys to enforce type
  on internal functionality (need to solve for custom `action`-like columns)
@zoobzio zoobzio changed the title Feat/818 generic table feat/818 generic table Mar 1, 2024
@zoobzio zoobzio changed the title feat/818 generic table feat(Table): 818 generic table Mar 1, 2024
@zoobzio zoobzio changed the title feat(Table): 818 generic table feat(Table): generic table Mar 1, 2024
@benjamincanac
Copy link
Member

I'm sorry you went through the trouble of refactoring all of this but this will be part of the v3 migration (#1289) which will change a lot of other things. I will not merge this in this version.

@zoobzio
Copy link
Contributor Author

zoobzio commented Mar 4, 2024

That's fine, I mostly wanted to provide an example of how a generic implementation might look within the context of the library to advocate for it's inclusion in v3 and offer my assistance in it's development. I would like to see every component take advantage of generics where it makes sense.

I probably could have just linked the branch in the issue instead of opening a PR, let me know if there is a better way to share proof-of-concept code like this and I will use that method in the future.

@zoobzio zoobzio closed this Mar 4, 2024
Copy link
Member

Thanks for providing an example, I'll keep this in mind when refactoring the components 😊

@Youhan
Copy link
Contributor

Youhan commented Mar 7, 2024

I appreciate the work on Genetic types πŸ™

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.

[Table] Generic component
4 participants