Skip to content
This repository has been archived by the owner on Apr 2, 2019. It is now read-only.

No way to "pre-process" a value before formatting without overwriting render #263

Closed
machineghost opened this issue Jun 28, 2013 · 9 comments

Comments

@machineghost
Copy link

Backgrid's built-in cell system works great as long as the value stored in the model's attribute is exactly what you want to send to your formatter. But what do you do if you need to process that value somehow before you format it?

For instance, let's say you have a type of cell where the model's value is an ID of an object. To display the relevant details of that cell, you need to get the object. This could be as simple as:

var id = this.model.get(this.column.get("name"));
var value = someCollection.get(id);

or it could be more complicated:

var someCollection;
if (id.indexOf('foo') != -1) {
    someCollection = collectionA;
} else if (id.indexOf('bar') != -1) {
   someCollection = collectionB;
} else if ...

var value = someCollection.get(id);

You could do all that logic in the formatter function, but then if you want to have different formatters for different cells of this type, you have to repeat the code. Alternatively you could overwrite the render method, but since there's other stuff in the render method, you risk having a future version of Backgrid change the render method somehow and break your code.

All of this can be solved very simply by just separating out the value-grabbing logic of Backgrid.Cell.prototype.render in to one or two methods:

getValue: function() {
    return this.model.get(this.column.get("name"));
},
render: function() {
    this.$el.empty();
    this.$el.text(this.formatter.fromRaw(this.getValue()));
    ...

(which would allow people to override getValue to "pre-process" their values), or even better:

getRawValue: function() {
    return this.model.get(this.column.get("name"));
},
processValue: function(rawValue) {
    return rawValue;
}
render: function() {
    this.$el.empty();
    this.$el.text(this.formatter.fromRaw(this.processValue(this.getRawValue())));
    ...

(which would let people override processValue without having to repeat the getRawValue logic).

Either one of these changes would allow a lot more flexibility in controlling the output of a Backgrid cell, would require almost no effort at all to implement, and would have no effect on any existing Backgrid code. Would it be possible to make one of the two part of Backgrid?

@wyuenho
Copy link
Contributor

wyuenho commented Jun 28, 2013

Mmm. Can you give a real life/concrete example of the kind of preprocessing you are talking about? Are you trying to use Backgrid to display nested/relational data?

@wyuenho wyuenho closed this as completed Jul 7, 2013
@machineghost
Copy link
Author

I did provide a real-life/concrete example: I have a bunch of rows of data, and some of the cells in those rows are IDs of objects. Rather than display the ID in the table, I want to display the name of the object that ID represents. I could go in to more particulars of my specific situation, but they're not particularly relevant.

This doesn't strike me as a far-out case; is it really that odd to have supplementary data outside of your collection?

@kriswill
Copy link

kriswill commented Jul 8, 2013

I have a similar requirement, probably more complex than yours. For example, I have a model with id/name/etc, but I need to render pop-over bubbles on hover and join the id with another collection of models that have detail data in them. So, I have 2 different ways I've approached it.

  1. look up the data in the other collection on cell render().
  2. look up the data on model.parse().

Option 1 is by far my favorite, but sometimes Option 2 is required when performing complex sorts.

@wyuenho wyuenho reopened this Jul 9, 2013
@wyuenho
Copy link
Contributor

wyuenho commented Jul 9, 2013

Assuming your API is arranged using the active record pattern, I think the cleanest way to approach this would be to have a translation layer that constructs a new UI model using the data just fetched from the server and add that newly constructed UI model to a UI collection. In essence, something like this:

var ServerUser = Backbone.Model.extend({
  url: "/users"
});

var ServerUsers = Backbone.Collection.extend({

  model: ServerUser,

  gridUsers: new Backbone.Collection(),

  initialize: function () {

    var self = this;

    this.on("add", function (model) {
      var newModel = new Backbone.Model();
      // put your translation logic here...
      newModel.set("id", someOtherIdNameMappingCollection.get(model.id).get("name")));
      // newModel.set() ....
      self.gridUsers.add(newModel);
    };

    // similarly convert the changes to the UI collection back to the server model
    this.listenTo(this.gridUsers, "change", function () {});

    // listen to "remove", "reset" and all the relevant events...
  }
});

var serverUsers = new ServerUsers();

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

serverUsers.fetch();

@wyuenho
Copy link
Contributor

wyuenho commented Jul 10, 2013

I just updated the gist to correctly reference the serverUsers.gridUsers.

@kriswill
Copy link

That's a nice strategy also! My only concern with this is saving/updating the model from the grid, since it's just a plain Backbone.Model, but I think you can easily address that by using an extended Model type. What I've been doing is using "look-up" methods on extended versions of my models for the collection I'm using for Backgrid. This way the original Model is still in play when bound to the grid, since I have dialogs and other UI elements that can affect their content, and I would want the grid to react to those changes.

@machineghost
Copy link
Author

So, what you suggested will work. But, when you compare it to the alternative, I don't see any advantage.

For one thing, your proposed solution is significantly more awkward; here's your version:

var ServerUsers = Backbone.Collection.extend({

    model: ServerUser,

    gridUsers: new Backbone.Collection(),

    initialize: function () {

      var self = this;

      this.on("add", function (model) {
        var newModel = new Backbone.Model();
        // put your translation logic here...
        newModel.set("id", someOtherIdNameMappingCollection.get(model.id).get("name")));
        // newModel.set() ....
        self.gridUsers.add(newModel);
      };

      // similarly convert the changes to the UI collection back to the server model
      this.listenTo(this.gridUsers, "change", function () {});

      // listen to "remove", "reset" and all the relevant events...
    }
  });

Here's a version using a hypothetical getValue method instead:

Backgrid.Extension.UserCell = Backgrid.Cell.extend({
    getValue: function(userId) {
        var userId = Backgrid.Extension.protrotype.getValue.call(this);
        return someOtherIdNameMappingCollection.get(userId).get("name")
    }
});

serverUsers.fetch();

Now the lack of boilerplate code makes it a little bit of an "apples and orangs" comparison, butdon't you think think the latter is shorter and easier to read than the former?

But beyond just that, what you're proposing has to be done on a per grid basis. BackGrid provides this great way to create new Cell subclasses, which can be used across your site by multiple grids. If there were a processValue or getValue this mechanism could be used to define a "UserCell" once. In contrast, your proposed solution has to be implemented separately for every Grid instance.

Finally, just to be clear, I'm asking for extremely minimal changes to BackGrid; literally just 6 lines of diff, all of which are purely refactoring changes:

-render: function () {
-    this.$el.empty();
-    this.$el.val(this.formatter.fromRaw(this.model.get(this.column.get("name"))));
+getValue: function() {
+    return this.model.get(this.column.get("name"));
+},
+render: function() {
+    this.$el.empty();
+    this.$el.text(this.formatter.fromRaw(this.getValue()));

(or just a few more lines to go with the "processValue" alternative).

@wyuenho
Copy link
Contributor

wyuenho commented Jul 11, 2013

@machineghost I understand your frustrations. I've been there, done that to other project owners. But let me reiterate one of Backgrid's goal - Backgrid is an attempt to provide a minimal set of widgets where people can easily customize. The corollary is Backgrid will not and does not solve all your grid problems. It's not about how many lines you add to Backgrid, it's about how useful those lines are to not just 2 people, but all the 100s of them out there that are using Backgrid at the moment. Rare, mission-specific use cases are left to the end-users to solve.

Let's review your proposed changes now.

Pros:

  • 6 line diff
  • Backward compatible

Cons:

  • Conceptual overlap.
    Formatters are designed specifically for the purpose of converting between model values and display values. Adding getValue, processValue, preprocessValues.... etc confuses me. I assume most casual users will feel the same because the intended purposes of these new methods will not be obvious. There's already a formatter, that's already a kind of processing, why are there more of them? This is against the Zen of Backgrid ™️ - There should be one-- and preferably only one --obvious way to do it.
  • Only good for read-only grids.
    What do you do after a new value is saved? Don't you still need to convert it back to whatever is appropriate? Your proposed change does not solve that problem.
  • Backgrid is minimal for a reason, if I keep adding features that will only be useful to 1 or 2 people at a time, pretty soon it won't be minimal anymore. For everything I add to Backgrid, I'll have to write tests, maintain them and make sure they don't break anything or stop other things from happening. This is especially true for cross-cutting changes to Cell.

That said, what I could do is to add to the CellFormatter signature:

fromRaw: function (rawValue, model) {
  // do whatever you want
},

toRaw: function (formattedValue, model) {
  // do whatever you want
}

Pros:

  • 2 line diff
  • Backward compatible
  • Useful for both read and write
  • No conceptual overlap

Cons:

  • I still have to maintain it, though I'm a-OK with this if the change is useful for more people.

@machineghost
Copy link
Author

Ok, first off:

Backgrid is an attempt to provide a minimal set of widgets where people can easily customize. The corollary is Backgrid will not and does not solve all your grid problems.

Amen. One of the key reasons why we chose to adopt BackGrid (after leaving a certain Hungarian notation-using library which will remain nameless) was because of how easy it is to customize. We have a complex site, and NO library could ever include everything we'd need.

I certainly wouldn't want to detract from that philosophy; quite to the contrary, enabling customization is the whole reason why I'm advocating for this ticket :-) And since your suggestion allows for the same customization as my original proposal, I think it's an excellent idea.

The one upside to my original idea was that it could be done once (vs. needing to do it once in the "to" and once in the "from"), but that can be solved by making your own processValue method on your formatter and having the to/from methods call it.

Thanks for taking the time to address this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants