cloudflarearchive / backgrid

Finally, an easily stylable semantic HTML data grid widget with a Javascript API that doesn't suck.
http://backgridjs.com
MIT License
2.01k stars 324 forks source link

Double sort indicators #453

Open ErikEvenson opened 10 years ago

ErikEvenson commented 10 years ago

If you sort by one column, and then another, the sort indicator for the first column does not go away -- even though it is not meaningful.

Example: screen shot 2014-02-25 at 5 14 12 pm

wyuenho commented 10 years ago

How to produce this? The examples on the docs have no such issue.

ErikEvenson commented 10 years ago

I'm not sure how to summarize...but here is the key part of my code that generates the Backgrid columns:

        if not _.contains(excludeColumns, "esi_id")
          columns.push
            label     : "ESI ID/Meter number"

            cell      : class extends Backgrid.ResourceLinkCell
              resourceName: "meters"
              useModel: true

            editable  : false

            formatter : class extends Backgrid.CellFormatter
              fromRaw: (rawValue, model) ->
                model.get("esi_id")

            name      : "esi_id"
            sortable  : true

        if not _.contains(excludeColumns, "meter_type")
          columns.push
            label     : "Meter type"
            cell      : "string"
            editable  : false

            formatter : class extends Backgrid.CellFormatter
              fromRaw: (rawValue, model) ->
                model.get("meter_type_display")

            name      : "meter_type"
            sortable  : true
wyuenho commented 10 years ago

What's the type of esi_id, why do you have a formatter for esi_id and what's in ResourceLinkCell?

ErikEvenson commented 10 years ago

esi_id is a string. It looks like I can drop the formatter.

Here is ResourceLinkCell:

  class Backgrid.ResourceLinkCell extends Backgrid.UriCell
    formatter : class extends Backgrid.CellFormatter
      fromRaw: (rawValue, model) ->
        if rawValue? then rawValue.name else ""

    getHref: ->
      if @useModel
        "##{@resourceName}/#{@model.id}"
      else
        rawValue = @model.get(@column.get("name"))
        "##{@resourceName}/#{rawValue.id}"

    render: ->
      throw new Error "No resource name found." unless @resourceName
      @$el.empty()
      rawValue = @model.get(@column.get("name"))

      if rawValue
        formattedValue = @formatter.fromRaw(rawValue, @model)

        @$el.append(
          $("<a>", {
            tabIndex: -1
            href    : @getHref()
            title   : @title || formattedValue
            target  : @target
          }).text(formattedValue)
        )

      @delegateEvents()
      @

    target: "_self"
    useModel: false
wyuenho commented 10 years ago

Which version of Backgrid are you using? Is there any errors in your console?

Just guessing blindly here, you need to provide a sortValue to your column definition because your model attribute is not flat. Backgrid by default just uses model.get(columnName) to get your model value for comparison. I suspect there's an error in the comparator that's stoping Backgrid from removing the sort caret.

http://wyuenho.github.io/backgrid/api/index.html#!/api/Backgrid.Column-cfg-defaults

ErikEvenson commented 10 years ago

I'm on Backgrid 0.3.5 and I'm not getting any errors. I'll look closer at providing a sortValue. Thanks!

wyuenho commented 10 years ago

Did sortValue solve ur problem?

ErikEvenson commented 10 years ago

Haven't gotten a chance to address. Will comment when I do on #426.

ErikEvenson commented 10 years ago

Sorry I took so long to get back to this. I created a simplified example to demonstrate the issue. It includes two basic columns using "flat" model attributes. I still get double carets. Here are the column definitions:

    if not _.contains(excludeColumns, "other_measure")
      columns.push
        label     : "other measure"
        cell      : "integer"
        editable  : false
        name      : "other_measure"
        sortable  : true

    if not _.contains(excludeColumns, "square_footage")
      columns.push
        label     : "square footage"
        cell      : "integer"
        editable  : false
        name      : "square_footage"
        sortable  : true

Since these are model attributes, I didn't add any sortValue.

Here is what gets rendered:

screen shot 2014-03-07 at 6 56 53 pm

peibol16 commented 10 years ago

Download this example and set other columns sortable to reproduce bug.

Is only with PageableCollection in server mode. I tried with master backgrid.js too.

http://backbone-paginator.github.io/backbone-pageable/examples/server-mode.html

I fixed it temporarily with commented on Issue #473

hoangpx commented 10 years ago

The same to me. It seem happen on server mode with backbone-pageable. Did u guys have any solution to fix this?

andrewdmoreno commented 10 years ago

I am also experiencing this same behavior under the same circumstances: PageableCollection in server mode. Any update on this one?

merusso commented 10 years ago

The problem, as peibol16 pointed out, looks to be that server-side PageableCollection doesn't trigger a "sort" event. I'm not sure if this is a bug there, but assuming that's the proper behavior, a fix would be to manually trigger this event in the Backgrid.Body sort handler:

collection.trigger("sort");

Pretty simple fix, but I don't know if it has ramifications on any other use of the grid.

kashifshamaz21 commented 10 years ago

+1 .. Same use case: Backgrid rendering a PageableCollection in server-side mode.. Sort carets from other comulns don't clear up... @wyuenho Any fix in the pipeline for this? thanks.

wyuenho commented 10 years ago

@peibol16 's fix seems to work. Would be nice if someone can send over a PR + tests.

merusso commented 10 years ago

@wyuenho Is it better to trigger the "sort" event from backgrid or to fix this behavior in Backbone.PageableCollection? I don't mind doing a pull request to fix this. peibol16's solution was a bit redundant, setting the column direction twice.

merusso commented 10 years ago

@wyuenho I initiated a pull request for this fix: https://github.com/wyuenho/backgrid/pull/499

merusso commented 10 years ago

@wyuenho I sent over a PR as you suggested, could you give an update on getting this merged in? Is there anything else you need?

madebydavid commented 10 years ago

I attempted to use your fix @merusso without success.

For any other's wanting a fix for double sort arrows when using PageableCollection in server mode. I used this little hack in the view containing the grid:

events: {
    'click th a': function(e) {
        /* remove the arrows on the other elements */
        $('th', $(this.el))
            .not($(e.target).parent())
                .removeClass('descending')
                .removeClass('ascending');
    }
}

Does anyone have a better alternative?

wyuenho commented 10 years ago

cc @ErikEvenson

brandonhall commented 10 years ago

Below is my solution to this issue. It's a bit hacky and relies on model.cid but it should be safer than direct DOM manipulation. Setting the direction to null will reset the state of the other columns and remove classes at the same time.

var grid = new Backgrid.Grid({
  columns: this._columns,
  collection: this.collection,
});

// Set direction on other columns to null
grid.collection.on('backgrid:sort', function(model) {
  // No ids so identify model with CID
  var cid = model.cid;

  var filtered = model.collection.filter(function(model) {
    return model.cid !== cid;
  });

  _.each(filtered, function(model) {
    model.set('direction', null);
  });
});
brandondurham commented 9 years ago

Nice little fix, @brandonhall . Using this until an official patch is implemented.

dougmatthews commented 9 years ago

works for me also, thanks @brandonhall. but I don't have the sort caret on initial page load, anyone have a fix that solves this?

ziodave commented 8 years ago

thanks @brandonhall ! I am using your fix w/ some trivial syntax changes in a subclass of Backgrid.Grid:

    this.listenTo( this.collection, "backgrid:sort", function (sort) {

        sort.collection.chain()
            .filter( function ( model ) {
                return model.cid !== sort.cid;
            } )
            .each( function ( model ) {
                model.set( "direction", null );
            } );

    } );
saverio-kantox commented 8 years ago

The second grid on the backgridjs home page (http://backgridjs.com/#complete-example) has the same issue, without any pageable collection behind.