Skip to content

port DataTable functionality from Pro #102

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

Merged
merged 4 commits into from
Mar 12, 2025
Merged

Conversation

kiprasmel
Copy link
Contributor

@kiprasmel kiprasmel commented Mar 6, 2025

problems:

  • cannot test in oss dashboard yet
    • build use-case with storybook & see if works & just release
  • importing svg images not possible?
    • added icon injection mechanism, let me know your thoughts

@kiprasmel kiprasmel marked this pull request as ready for review March 11, 2025 06:40
@kiprasmel kiprasmel requested a review from Cahllagerfeld March 11, 2025 06:40
@kiprasmel kiprasmel force-pushed the port-datatable-from-pro branch from 54e2710 to cfb7131 Compare March 11, 2025 06:43
@kiprasmel kiprasmel force-pushed the port-datatable-from-pro branch from cfb7131 to 1694673 Compare March 11, 2025 06:44
Comment on lines +121 to +122
sorting={sorting}
onSortingChange={setSorting}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think sorting here actually works. any idea what's missing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a useSorting hook involved, that is missing here I think 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that hook in pro is only for syncing the sorting state to the router/url. i don't think it's needed here because it's specific for our use case. or did i miss something and it has logic that makes the sorting work?

i checked it again, i see that the state gets set in the url and then taken in & applied. but can't we have the sorting work w/ local state first, instead of putting it in the url? at least for the example here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I'm not sure tbh. The difference might be that in Pro were doing server-side sorting, while here you want to rely on client-only sorting https://tanstack.com/table/v8/docs/guide/sorting#client-side-vs-server-side-sorting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm that's tough. because in the storybook demo it's not working at all. i'll take a look at how to fix this, but in the meantime i think we can merge, since it'll work fine in our repos?

Comment on lines +121 to +122
sorting={sorting}
onSortingChange={setSorting}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a useSorting hook involved, that is missing here I think 🤔

Copy link
Collaborator

@Cahllagerfeld Cahllagerfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storybook was working for me besides the sorting that you left int he comment.

I think once we figured it out, we should give it a shot 👍

@kiprasmel kiprasmel force-pushed the port-datatable-from-pro branch from 1b28f7f to 2faf23b Compare March 12, 2025 15:44
@kiprasmel
Copy link
Contributor Author

sorting to be fixed in the example later

@kiprasmel kiprasmel merged commit 621a79e into main Mar 12, 2025
2 checks passed
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.

2 participants