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

Should a 'table' component sort it's data in React/Flux arch? #1

Open
sterpe opened this issue Dec 23, 2014 · 3 comments
Open

Should a 'table' component sort it's data in React/Flux arch? #1

sterpe opened this issue Dec 23, 2014 · 3 comments

Comments

@sterpe
Copy link

sterpe commented Dec 23, 2014

More of a conceptual question, really. It feels like sorting should be done at the store layer which then fires the change event, and the component re-renders the data from the stores get method. I think it will be easier to work in pagination and other real-world table requirements if react-table approaches it that way: i.e., clicking on a column/sort arrow dispatches an event that informs the relevant store what column/orientation to sort on, etc, leaving those implementation specific details to the user.

@NickTomlin
Copy link
Owner

@sterpe good question! This could work if you had specific store methods that sorted the data based on criteria, and may be a better top level solution. I don't think the sort should broadcast a change event though, since components probably only want to know when the data itself has changed, and may want different sorted representations of the data.

I think it's acceptable to keep the sorting logic within the table for most cases, but it might be interesting to split off the sort related methods and provide the ability for them to be mixed in to a store (or maybe just a top level component?) of need be. It would just make the integration a lot more difficult and require the user to conform to a certain application structure. What do you think?

Regarding pagination, at the moment could be handled by limiting the data passed through the data prop, but you would need to build build all of the paging functionality yourself :|. I'd love to add this in the future, perhaps through a mixin? Feel free to open another issue/PR regarding pagination.

@sterpe
Copy link
Author

sterpe commented Dec 26, 2014

It still strikes me that the approach of implementing sorting within the component itself feels a little more comfortable in Backbone.

I hope this doesn't seem like a trivial nitpick about implementation details, but feels like a change in ordering is a data change that should be scheduled via the dispatcher to some store in the typical way, then the freshly reordered table data is passed down as new props to a table component. It's my understanding that in general props should be read-only, though at the moment I can't find that stated explicitly in any React documentation, however the component life-cycle methods strongly seem to suggest it -- at least to my reading.

@NickTomlin
Copy link
Owner

I apologize for the late response!

You're not being picky at all, I really appreciate the feedback :)

I guess I don't see sorting as breaking the read-only contract of a child component, instead it is just a display of the same data handed down by the parent. This display can be arbitrarily sorted without affecting the sort order of the original data, but any changes to the actual data would go through the normal flux lifecycle, e.g: allowing a user to delete a row by clicking on it would trigger an action that would dispatch, effect the store, and re-render the table.

This also allows multiple components to sort the data without conflict. If the store was in charge of sorting the data all subscribers would be forced to accept whatever the sort order of the store.

That said, I think it may be worthwhile to provide a way to disable the sorting feature, and just trigger a handler. That way if you wanted to implement your own top-down sorting you could. What do you think?

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

No branches or pull requests

2 participants