Use tanstack table for Competitors #10778
Conversation
There was a problem hiding this comment.
Okay, I see your point much more clearly now. The configuration is indeed cumbersome, and especially in terms of footer counts the setup is much more complicated than I anticipated.
I'll leave it up to you whether you want to use it and merge this PR or if you prefer to discard it.
| ) : ( | ||
| info.getValue() | ||
| ), |
There was a problem hiding this comment.
I don't get this with regards to the ternary condition. If getValue is truthy, you return an <a> link, but if it's falsey you just return... that falsey value explicitly? Why not just info.getValue() && (<a>)?
There was a problem hiding this comment.
Pretty sure I just put the wrong condition, it should be checking that the wca id exists.
There was a problem hiding this comment.
In that case info.getValue() ? is definitely not right, because that cell is linked to user.name as accessor.
| const { countryCount } = getTotals( | ||
| info.table.getCoreRowModel().rows.map((row) => row.original), | ||
| eventIds, | ||
| ); |
There was a problem hiding this comment.
Hm, this is indeed quite unintuitive.
There was a problem hiding this comment.
Two things here:
getTotalsis a helper function which is shared with the psych sheet which is using the original registrations data. So to keep it as a shared helper function I had to pass in the original rows. That helper could be modified though, to take in the tanstack table data in some other format.- I made the cell value just the country name, to get automatic sorting capabilities from tanstack table. I could instead make the cell value be the country object. That would make getting the flag classname simpler, and possibly make the footer calculation easier. But, then a custom sort function would need to be passed in.
Even with those changes, the footer would still not be the most intuitive thing. And this just highlights that there are so many ways to approach tables that it will be hard for us to be consistent across the codebase - especially with non-WST contributors.
The existing (non-tanstack) code is quite easy to understand, and you can explicitly see the data being sorted/etc. If you want to update the footer, it's easy you just go to the footer component and that component has the data immediately available.
As I mentioned elsewhere, I really like that the column definitions centralize the header/body/footer of a column in one spot. But that's about it. The automatic sorting/filtering can be nice - but not if the cell values are 'complicated, such as names with links or countries with flags, etc.
So I'm undecided whether I want to finish off this PR, or cancel it. 🤷
There was a problem hiding this comment.
Hm, I admit I might have been seduced by the notion of a formless library a bit. But now I definitely see your points...
The bright side here is that Tanstack has a pretty active GitHub account, including Discussions (as in, the GitHub feature called "Discussions"). Could I compel you to post a summarized version of your thoughts there, and see what people think? I feel like our use-case is totally not unreasonable, and especially the cases of
- "complex" values for a cell (i.e. country name but also flag, person name but also link based on ID)
- footer counts / totals
would be interesting to debate.
If an answer is forthcoming in the next few days, you can pick it up for this PR. Otherwise, I suppose we'll just put Tanstack-Table into the drawer again for the time being.
| } | ||
| }, | ||
| ...eventIds.map((id) => ({ | ||
| id, |
There was a problem hiding this comment.
Maybe something like event-${id}?
| ))} | ||
| </Table.Header> | ||
| <Table.Body> | ||
| {table.getSortedRowModel().rows.length > 0 ? ( |
There was a problem hiding this comment.
Can you return early if no columns are found?
There was a problem hiding this comment.
Do you mean rows? Returning early would omit the header/footer. That could work, I guess. I prefer having the header/footer, and I think many of our other tables do that, including as the psych sheet.
See slack discussion about my concerns.