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

Multi column sort support #957

Closed
wants to merge 3 commits into from
Closed

Multi column sort support #957

wants to merge 3 commits into from

Conversation

CzBuCHi
Copy link

@CzBuCHi CzBuCHi commented Jan 8, 2018

original PR: #946

  • splitted simple and multi-sort into separate components, which internally render original table component.
  • original table no longer gets sortBy and sortDirection via props (using internal fn for that)
  • original table no longer calculate arguments for sort function (again delegated to internal fn)

@CzBuCHi
Copy link
Author

CzBuCHi commented Jan 10, 2018

@bvaughn can u please review this version? I think it could be final one, but also welcome any suggestions whats can be improoved ... thx

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

This has a bigger footprint than what I had in mind. I've created a counter-proposal (see PR #966) that shows the direction I tried to suggest when we last chatted about this. Please let me know your thoughts?

@bvaughn
Copy link
Owner

bvaughn commented Jan 13, 2018

I don't think I'm comfortable with the amount of duplication in prop-types and public methods between SimpleTable, MultiTable, and Table. Let's continue discussion this feature on PR #966 and see if we can find a solution with a smaller footprint that will still enable you to get multi-sort working. 😄

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