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 filterFields to Sorting #162

Merged
merged 1 commit into from
Jun 8, 2017

Conversation

Vanderslice
Copy link
Contributor

@Vanderslice Vanderslice commented May 16, 2017

Purpose: We want to be able to pass an object containing fields for filtering, like search terms, to the sort to allow for remote sorting to maintain those filters.

Concerns:

  • Just wanted to get a quick pair of eyes on the changes
  • Not sure if I should move this to the Column Manager's plugins
  • Wanted to get another opinion on referring to these as filterFields vs filterParams

@codecov-io
Copy link

codecov-io commented May 16, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@9e1b8f1). Click here to learn what that means.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #162   +/-   ##
=========================================
  Coverage          ?   76.87%           
=========================================
  Files             ?      112           
  Lines             ?     4558           
  Branches          ?      275           
=========================================
  Hits              ?     3504           
  Misses            ?      847           
  Partials          ?      207
Impacted Files Coverage Δ
src/components/core/ColumnManager.js 94.93% <ø> (ø)
src/components/layout/FixedHeader.jsx 50.68% <ø> (ø)
src/actions/GridActions.js 76.7% <0%> (ø)
src/components/Grid.jsx 80.64% <100%> (ø)
src/components/layout/header/Column.jsx 79.41% <83.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e1b8f1...f8a26c8. Read the comment docs.

@bencripps
Copy link
Owner

Can you explain the use case a little more, and perhaps provide an example?

@Vanderslice
Copy link
Contributor Author

Vanderslice commented May 17, 2017

Use Case: We have a search field that allows for filtering records in the grid and we want to maintain that filter when remote sorting. Unless I'm overlooking something, we currently don't have a way to pass along those extra values we want to be maintained to the the remoteSort so when you sort it returns the unfiltered data.

Our setup is:
Container
-Nav/Search
-Grid - React-Redux-Grid

  1. When we search we're storing that as an object in the state which is used as a query string on our GETs.
  2. When we remoteSort it doesn't have access to the search term
  3. The GET is done without our search term as a query string

Copy link
Owner

@bencripps bencripps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changeset looks good. 1 thing I'd like changed (see comment). I'd also like to see filterFields added to the readme. Would you mind adding that?

return dataSource(
{pageIndex},
filterFields,
sortParams
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This becomes a breaking change if filterFields is ordered before sortParams, whereas it's probably just a minor change if it comes second. Do you think it's OK if we keep the order as it was before, appending the new arg?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to include group pageIndex and pageSize as a pagingParams object to match pager's setPageIndexAsync's pattern: dataSource({ pageIndex, pageSize }, filterFields, sortParams)

Copy link
Contributor Author

@Vanderslice Vanderslice May 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be a breaking change to either doRemoteSort or setPageIndexAsync. I chose to change doRemote sort since it seems likely that doRemoteSort will have the smallest impact and the pattern for setPageIndexAsync seems more self explanatory.

@bencripps
Copy link
Owner

Changes look good, would you mind updating the docs?

Passing filterFields and pageSize to ColumnManager
@bencripps bencripps merged commit eee6a75 into bencripps:master Jun 8, 2017
@bencripps bencripps changed the title [WIP] Added filterFields to Sorting Added filterFields to Sorting Jun 9, 2017
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.

3 participants