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

Using external sort is broken in 0.6.1 #453

Closed
mwtorkowski opened this issue Jul 7, 2016 · 13 comments
Closed

Using external sort is broken in 0.6.1 #453

mwtorkowski opened this issue Jul 7, 2016 · 13 comments

Comments

@mwtorkowski
Copy link

In 0.6.1, when using external data and sorting capabilities, the sort ascending / descending component never renders as the checks rely on comparing that.state.sortSettings.sortDirection to 'asc' or 'desc'.
Problem is, the sortDirection key is internal value, normally set by griddle when NOT using external sorting. There's no way to set it from the outside, hence sorting component never renders (it thinks there's no sorting applied at all).

@DmitryOlkhovoi
Copy link

DmitryOlkhovoi commented Jul 12, 2016

Yeap i have the same issue here
#449

@ryanlanciaux

@ryanlanciaux
Copy link
Member

Sorry your encountering this -- in regard to #449, is this only happening with External sorting?

@fechy
Copy link

fechy commented Aug 4, 2016

+1

I did some digging:

Because of the return in here:

if (this.props.useExternal) {
    this.props.externalChangeSort(column, this.props.externalSortColumn === column ? !this.props.externalSortAscending : true);
    return;
}

This other part is never reached:

var sortDirectionCycle = columnMeta.sortDirectionCycle ? columnMeta.sortDirectionCycle : [null, 'asc', 'desc'];
 var sortDirection = null;
 // Find the current position in the cycle (or -1).
var i = sortDirectionCycle.indexOf(this.state.sortDirection && column === this.state.sortColumn ? this.state.sortDirection : null);

// Proceed to the next position in the cycle (or start at the beginning).
i = (i + 1) % sortDirectionCycle.length;

 if (sortDirectionCycle[i]) {
            sortDirection = sortDirectionCycle[i];
 } else {
            sortDirection = null;
}
 var state = {
            page: 0,
            sortColumn: column,
            sortDirection: sortDirection
};

 this.setState(state);

Therefore, sortDirection is always undefined, so this part in gridTitle.jdx fails:

if (that.props.sortSettings.sortColumn == col && that.props.sortSettings.sortDirection === 'asc') {
      columnSort = that.props.sortSettings.sortAscendingClassName;
      sortComponent = that.props.useGriddleIcons && that.props.sortSettings.sortAscendingComponent;
 } else if (that.props.sortSettings.sortColumn == col && that.props.sortSettings.sortDirection === 'desc') {
      columnSort += that.props.sortSettings.sortDescendingClassName;
      sortComponent = that.props.useGriddleIcons && that.props.sortSettings.sortDescendingComponent;
}

Hope this helps

@DmitryOlkhovoi
Copy link

@ryanlanciaux Could you pls fix it?

@albert-tse
Copy link

someone please make a pull request for this fix

@umchee
Copy link

umchee commented Aug 31, 2016

+1

1 similar comment
@M3lkior
Copy link

M3lkior commented Sep 7, 2016

+1

@aclowes
Copy link

aclowes commented Sep 23, 2016

I worked around this with a subclass and patch hack. I'm sure there are better ways but might be useful for someone.

import Griddle from 'griddle-react';

export default class Grid extends Griddle {
  oldGetSortObject = this.getSortObject;
  getSortObject = () => {
    const sortObject = this.oldGetSortObject();
    sortObject.sortDirection = sortObject.sortAscending ? 'asc' : 'desc';
    return sortObject;
  };
}

@M3lkior
Copy link

M3lkior commented Sep 27, 2016

thanks @aclowes , it works.

@twolfe2
Copy link

twolfe2 commented Nov 8, 2016

I got the sort icon working by using #486.

P.S. make sure you rebuild your project after switching versions

@ryanlanciaux
Copy link
Member

Has anyone had more success with 0.7.0?

@flanamacca
Copy link

Confirming - upgrading to 7 fixed this for me

@ryanlanciaux
Copy link
Member

Thanks for checking -- Will close this out for now then :)

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

10 participants