React-virtualized: defaultTableHeaderRenderer should include the container

[silvenon]

I've been trying to reorder table columns with react-sortable-hoc. This is what I came up with, it almost works well, but I have to drag the headers pretty far in order for them to swap places.

It turns out that this happens because the defaultTableHeaderRenderer actually returns an array of children, without the container, so I have to add the container myself. However, this means that column header decorated with sortableElement won't be a _direct_ child of the header row decorated with sortableContainer, and that seems to be what causes the problem.

My proposition is to adjust defaultTableHeaderRenderer to also include the container, rather than only children. This may fix future issues with integrating with other library, not only react-sortable-hoc.

[bvaughn]

Thanks for the clear problem description. Here are my initial thoughts.

However, this means that column header decorated with sortableElement won't be a _direct_ child of the header row decorated with sortableContainer, and that seems to be what causes the problem.

What problem? Having to drag the header columns farther than you think is necessary?

My proposition is to adjustdefaultTableHeaderRenderer to also include the container, rather than only children.

This would be a breaking change and would require a new major release. I'm not sure _this_ is enough to justify the effort that would cause. If/when I do a new major version, I have bigger ideas I'd want to tackle first.

This may fix future issues with integrating with other library, not only react-sortable-hoc.

This claim seems a bit shaky (eg What other libraries? How do we know this would make things easier with them? etc.)

[silvenon]

You’re right. This sounds like a bug I should raise in react-sortable-hoc instead. My bad…

[silvenon]

For the record, perhaps it could be done in a non-breaking way, by exposing another renderer defaultTableHeaderContainerRenderer, but if the need for that hasn’t appeared before, no reason to add it now.

[bvaughn]

I appreciate your being so understanding about this. ❤️ Thank you.

[silvenon]

@bvaughn just so I know, would you accept a PR for that?

[bvaughn]

I'm willing to _review_ one but no promises that I'll accept it. Doesn't seem like a very intuitive prop name nor a common use-case. 😄

[silvenon]

Thanks for being willing to review it. In the meantime I realized that I'm not very smart 😆 and that the existing API provides enough flexibility. headerRowRenderer renderer allows me to make sortable header cells direct descendants of the their sortable container (the header row) via the columns param. Thanks for providing a flexible API! ❤️

If anyone reading this would like to see an example, I'll post it soon to react-sortable-hoc#218.

[bvaughn]

Great! 🎉 I'm glad you found a solution! Thanks for leaving an update here as well. 😄

[bvaughn]

You've probably already seen this but @clauderic added a demo of this with source code.

[silvenon]

Wohoo! 🌮