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

#977 Maintain filter state in buildColumns updates #1058

Closed
wants to merge 1 commit into from
Closed

#977 Maintain filter state in buildColumns updates #1058

wants to merge 1 commit into from

Conversation

alexcberk
Copy link

@alexcberk alexcberk commented Nov 8, 2019

Addresses Issue #977

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 77.34% when pulling ce277b4 on acberk:master into 2d37c38 on gregnb:master.

@gabrielliwerant gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Nov 22, 2019
@gabrielliwerant gabrielliwerant self-requested a review November 22, 2019 22:53
@gabrielliwerant
Copy link
Collaborator

Hey @AcBerk, thanks for looking into this. Just so I'm clear on what's being fixed while I'm testing, is it true that the filters are reset when filter state in the column options is manually updated? Or is there another cause?

@gabrielliwerant gabrielliwerant added the needs verification For issues that can't be reproduced or are otherwise unclear label Nov 26, 2019
@alexcberk
Copy link
Author

alexcberk commented Nov 27, 2019

@gabrielliwerant I'm not sure that's quite it.

The contrived example showed the filters resetting when any state was manually updated in the column options (in this case, the onRowsSelect function).
https://codesandbox.io/s/muidatatables-example-u6u8b

From testing, that state update triggers componentDidUpdate --> setTableData --> buildColumns

The stack trace:

  1. componentDidUpdate(prevProps) {
    if (this.props.data !== prevProps.data || this.props.columns !== prevProps.columns) {
    this.updateOptions(this.options, this.props);
    this.setTableData(this.props, TABLE_LOAD.INITIAL, () => {
  2. setTableData(props, status, callback = () => {}) {
    let tableData = [];
    let { columns, filterData, filterList } = this.buildColumns(props.columns);
  3. buildColumns = newColumns => {
    let { columnData, filterList, filterData } = this.state;
    columnData = columnData || [];
    filterData = filterData || [];
    filterList = filterList || [];

The filters are reset by any manual state call within the column options. It appears that the component updates when state is called. Is that intended? My fix passes the filter data through the buildColumns function so that any previous filters remain active on component updates.

@patorjk
Copy link
Collaborator

patorjk commented Dec 1, 2019

@AcBerk in the way the table currently works, all state is erased when the table re-renders - filters, sorting, search text, selected rows, etc etc (see this issue: #807). I've put in another PR that will cause state to persist during re-render if it's not being overwritten by inputted options or column data. You can see it here if you're interested: #1086

@alexcberk
Copy link
Author

@gabrielliwerant Did you have a chance to look at this? I've seen a lot of talk around derived state, but this smaller PR should take care of cases where selecting a row incorrectly resets the filters.

In other news, I found that I can work around it by self-managing filter state, and passing it down to each column with the filterList option. But this feels like a workaround.

@patorjk
Copy link
Collaborator

patorjk commented Jan 3, 2020

@AcBerk Using the filterList option is how you're currently supposed to preserve state with filters (see @gabrielliwerant's comment here: #807 (comment)). An in-depth discussion of the problem can be found in #807.

This PR seems to only address the filterList option, and it doesn't allow the user to override the filterList from the option's object once the table has created a filterList (which prevents developers from loading filters externally after the table is loaded). There is another PR #1086 that would make all options, including filterList, persist on rerender if not overwritten on the options object.

@alexcberk
Copy link
Author

@patorjk Fair enough, that makes sense. I don't think I knew about the filterList option during this fix. Sounds like that'll be the answer in the meantime. Thanks for chiming in.

@alexcberk alexcberk closed this Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Useful to mark PRs for what's up next to review needs verification For issues that can't be reproduced or are otherwise unclear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants