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

Added support for table multi-column sorting #946

Closed
wants to merge 3 commits into from
Closed

Added support for table multi-column sorting #946

wants to merge 3 commits into from

Conversation

CzBuCHi
Copy link

@CzBuCHi CzBuCHi commented Jan 5, 2018

Modified how sort method is called when used clicks on column header:

  • simple click: original behavior
  • shift + click:
    • on column, that is used for sorting: reverse column sort direction
    • on new column: add that column with default sort to the end of sort array
  • control + click:
    • if column is used for sorting: remove column from sort array, otherwise add to the end

if sorted only by one column return single value instead of array (backward comp.)

preview: (action shows how to get from state in one row above)

Action Sort definition
First ↑ Second Third First ASC
click First First ↓ Second Third First DESC
shift-click First First ↓ Second Third First DESC
control-click First First Second Third
click Second First Second ↑ Third Second ASC
shift-click First First ↑ Second ↑ Third First ASC then Second ASC
control-click First First Second ↑ Third Second ASC
First ↑ Second ↑ Third Second ASC then First ASC
click First First ↓ Second Third First DESC
shift-click Second First ↓ Second ↑ Third First DESC then Second ASC
control-click First First Second ↑ Third Second ASC

EDIT: Fixed copy-paste bugs in table

@bvaughn
Copy link
Owner

bvaughn commented Jan 6, 2018

My initial thought: This is a lot of complexity (the behavior you're describing) to be built into Table. This sort of logic might be nicer if packaged as something you could optionally add into a Table or its Columns, perhaps via the sort function property.

I believe the only thing preventing you from implementing it that way is the Table does not pass the click event to the sort function, like it does with onHeaderClick. If we added that parameter to sort, would this be an acceptable alternative approach for you?

I'm thinking of an API like this:

<Table
  {...props}
  sort={createComplexSort(yourSimpleSortFunction)}
/>

PS. I think your table has some typos in it, wrt what's being shift or control clicked. Either that or I'm just misunderstanding some of the rows. For example, how does shift-clicking Second end up adding First to the sort? And how does control-clicking Second remove First?

@bvaughn
Copy link
Owner

bvaughn commented Jan 6, 2018

I am not comfortable with changing the current default behavior of Table in the way proposed by this PR. I think an alternate implementation, which allows users to opt into the more advance sort behavior would be something I could feel comfortable about adding to the project though. If you would like to re-open this PR using an approach more like that, I'd be happy to review it.

@bvaughn bvaughn closed this Jan 6, 2018
@CzBuCHi CzBuCHi mentioned this pull request Jan 8, 2018
@CzBuCHi
Copy link
Author

CzBuCHi commented Jan 8, 2018

I am not comfortable with changing the current default behavior of Table in the way proposed by this PR. I think an alternate implementation, which allows users to opt into the more advance sort behavior would be something I could feel comfortable about adding to the project though. If you would like to re-open this PR using an approach more like that, I'd be happy to review it.

i will create new PR (dont have permission to open this one) - #957

I believe the only thing preventing you from implementing it that way is the Table does not pass the click event to the sort function, like it does with onHeaderClick. If we added that parameter to sort, would this be an acceptable alternative approach for you?

Thats would allow consumer to set sort by multiple columns, but table would render only for first one...

PS. I think your table has some typos in it, wrt what's being shift or control clicked. Either that or I'm just misunderstanding some of the rows. For example, how does shift-clicking Second end up adding First to the sort? And how does control-clicking Second remove First?

yea, these are copy-paste bugs ... fixed

@bvaughn
Copy link
Owner

bvaughn commented Jan 8, 2018

i will create new PR (dont have permission to open this one) - #957

Yes, thanks! That's what I meant (a new PR) 😄

My main goal is to not add this complexity into Table if it can be encapsulated in a small helper component/function. (This is a general pattern I've used in RV to try to keep things easier to maintain.)

Thats would allow consumer to set sort by multiple columns, but table would render only for first one...

I'm not sure I understand your wording here. I think you may be saying that because Table defines sortBy and sortDirection as string types, it wouldn't handle the fact that it might be sorted by multiple columns. To that end, I would not be opposed to changing the prop-type validations to make those string-or-array-of-strings.

@CzBuCHi
Copy link
Author

CzBuCHi commented Jan 8, 2018

My main goal is to not add this complexity into Table if it can be encapsulated in a small helpe component/function.

Okay ... i can do that ... but look how i did it in that second PR first (all multi-sort related code is enabled by single prop) - maily for another code suggestions i could use ...

I'm not sure I understand your wording here. I think you may be saying that because Table defines sortBy and sortDirection as string types, it wouldn't handle the fact that it might be sorted by multiple columns.

yea .. that was in my thought ... and my wording is wierd, because english is not my first language :(

@CzBuCHi
Copy link
Author

CzBuCHi commented Jan 9, 2018

My main goal is to not add this complexity into Table if it can be encapsulated in a small helper component/function.

looked tru the code and im not sure that composition is right way for this - i modified 'private' method _createHeader and added another 'private' method _handleMultiSortColumnClick to handle multi-sort.

i could create HOC which would override _createHeader but that is just wrong - HOC becomes aware about 'private' method of Table.

Any ideas how to do this?

@bvaughn
Copy link
Owner

bvaughn commented Jan 9, 2018

"Composition" is kind of a broad term. What I suggested was a utility function to create a complex sort callback prop to pass to Table. This wouldn't require any special knowledge of Table internals?

@CzBuCHi
Copy link
Author

CzBuCHi commented Jan 9, 2018

This wouldn't require any special knowledge of Table internals?

technically i can write sort function, that would sort by multiple columns right now - but table wouldn't know about it and didnt show correct sort indicators ...


Writing this gived me an idea:

  1. create new TableBase component - code copied from current Table component except how new sort order is calculated.
  2. current Table becomes wrapper around TableBase with current sort order calculations.
  3. create new MultiTable (TableEx?) as second wrapper around TableBase with multi-sort order calculations.

from external perspective behavior of Table component wouldn't change ...

@bvaughn
Copy link
Owner

bvaughn commented Jan 9, 2018

but table wouldn't know about it and didnt show correct sort indicators ...

Which is why I also suggested updating the sortBy and sortDirection props to support arrays-of-strings.

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