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 325 forks source link

No Way (That I Can Tell) To Change Target Attribute on UriCells #242

Closed machineghost closed 11 years ago

machineghost commented 11 years ago

When you use UriCell to generate an <a> tag, that tag automatically comes with a target="_blank". While I understand I can write my own cell to get any target attribute I might want, this seems like an awfully opinionated behavior for the base library to have. Would it be possible to add some sort of option to UriCell to set the target attribute, or failing that an option to remove it entirely?

kriswill commented 11 years ago

You could override the render method to use a class property of target for the target attribute:

Backgrid.UriCell = Backgrid.UriCell.extend({
  target: '_blank',
  render: function() {
    Backgrid.UriCell.prototype.render.call(this)
    this.$('a').attr('target', this.target)
    return this
  }
})
machineghost commented 11 years ago

Thanks @krisswill ... but as I noted "I understand I can write my own cell to get any target attribute I might want ...". It's not that a custom class isn't an option, as I'm happy to write such a sub-class if an in-library fix isn't viable. My point was more that BackGrid is a general purpose grid library, and that links that open in the same window have been around since the start of the web, so perhaps it would be an improvement to the library to support this use case instead of requiring custom code for it.

kriswill commented 11 years ago

I agree. There are many opinions that the default library has that aren't appropriate for certain use cases that I've encountered. I had to do a lot of sub-classing to tweak it's behavior. I would prefer that I didn't have to do it, but sometimes it can't be avoided.

If I were to speculate why it's like this, I think target='_blank' is a reasonable workaround, assuming you don't want to completely interrupt the Backbone application--since not having that would replace the current location. I have had to implement onbeforeunload handlers to prevent people from destroying their unsaved state when navigating away to static links, but this is very custom and messy and I wouldn't expect a Backbone app to have navigation counter-measures employed from a component like Backgrid.

If anything the UriCell probably should have some options passed thru from the column definition, that allows you to override the default target attribute of the link. How to do that in a tasteful and consistent way is the question.

machineghost commented 11 years ago

If I were to speculate why it's like this, I think ...

Totally; it makes complete sense that BackGrid uses target="_blank" as its default, as that is certainly the "safest" option.

How to do that in a tasteful and consistent way is the question.

One simple way would be to change this line (from UriCell's render):

target: "_blank"

to:

target: this.target || "_blank"

This would allow anyone who wanted to have in-window links to simply define a UriCell sub-class ...

Backgrid.Extension.InWindowUriCell = Backgrid.UriCell.extend({target: ''});

... which they could then use just like UriCell:

new Backgrid.Grid({
    columns: [{cell: 'inWindowUriCell', ...

With just a one line change, I think you get a pretty tasteful/consistent improvement; after all, something similar is done when one overrides orderSeparator in an IntegerCell subclass.

wyuenho commented 11 years ago

The UriCell's defaults seem to be a constant nuisance for a lot of folks. In particular, currently people have to override render to change the behaviors of the title, target attributes and the innerText the anchor renders.

Would folks be satisfied if UriCell adopts a similar strategy as the NumberCell and DateTimeCell subclasses? That way you can extend a UriCell in-place inside the column definition to configure a custom UriCell.

machineghost commented 11 years ago

I can't speak for anyone else, but as far as I'm concerned polymorphism is proven (both in Backgrid's NumberCell/DateTimeCell and just in programming in general), so it seems like the way to go with UriCell.

kim2014 commented 10 years ago

Sorry for the question but I would like to have the href different from title in UriCell. How to write the custom render to overwritten the backgrid's UriCell render. Again sorry I am very new to backgrid.

machineghost commented 10 years ago

First off, a quick suggestion: if you have questions about how to use a library, a site like StackOverflow.com is much more geared towards that sort of thing than a Github issues comment thread. That being said, as I understand it ...

Backgrid uses the cell's original value as the URL, and it uses the cell's title property or the formatted value for the title. So, if your cell's contents are the URL, things are easy. You can either:

1) (assuming your title and the link text are the same) make a formatter that "formats" the URL in to a title, or 2) provide a title as a property of the cell (eg. override initialize and set a title in it).

If your cell's contents aren't the URL, then you do have to override the render method. Basically just do:

Backgrid.Cell.extend({
    // Copy/paste Backgrid's render in here
    render: function () {
        this.$el.empty();
        var rawValue = this.model.get(this.column.get("name"));
        var formattedValue = this.formatter.fromRaw(rawValue, this.model);
        this.$el.append($("<a>", {
          tabIndex: -1,
          href: rawValue,
          title: this.title || formattedValue,
          target: this.target
        }).text(formattedValue));
        this.delegateEvents();
        return this;
  }

And then replace the line:

              href: rawValue,

with whatever you want the URL to be.

Personally I would prefer if Backgrid used a lot of smaller functions so that overriding stuff like this was easier. Imagine if there was a method on Cell:

getHref: function(rawValue) {
    return rawValue;
}

and Backgrid used it instead of the rawValue you directly:

    this.$el.append($("<a>", {
          tabIndex: -1,
          href: this.getUrl(rawValue),

Then instead of having to copy/paste BackGrid code in to your code, you could just overwrite getUrl:

Backgrid.Cell.extend({
    getUrl: function(rawValue) {
         var myCalculatedUrl = doSomethingWith(rawValue);
         return myCalculatedUrl;
    })

... but it doesn't, so (as far as I can tell) you're stuck with the copy/paste.

wyuenho commented 10 years ago

I'll introduce more smaller functions into the cell classes. It looks like lots of people are confused.

wyuenho commented 10 years ago

I was just waiting for the API to get stabilized, looks like they are now.

wyuenho commented 10 years ago

Also, if you really would like to, here's a much simpler solution:

render: function () {
  this.__super__.render.apply(this, arguments):
  this.$('a').attr({'href': 'whateveruwant'});
  return this;
};
kim2014 commented 10 years ago

Thanks a lot Jeremy My cell contents are not the URL(href) so I need to have the override render function as you mentioned below. Sorry for the basic question. Where I should place this override render function Thanks Kim

Sent from my iPhone

On Mar 2, 2014, at 10:41 AM, "Jeremy" notifications@github.com<mailto:notifications@github.com> wrote:

First off, a quick suggestion: if you have questions about how to use a library, a site like StackOverflow.comhttp://StackOverflow.com is much more geared towards that sort of thing than a Github issues comment thread. That being said, as I understand it ...

Backgrid uses the cell's original value as the URL, and it uses the cell's title property or the formatted value for the title. So, if your cell's contents are the URL, things are easy. You can either:

1) (assuming your title and the link text are the same) make a formatter that "formats" the URL in to a title, or 2) provide a title as a property of the cell (eg. override initialize and set a title in it).

If your cell's contents aren't the URL, then you do have to override the render method. Basically just do:

Backgrid.Cell.extend({ // Copy/paste Backgrid's render in here render: function () { this.$el.empty(); var rawValue = this.model.get(this.column.get("name")); var formattedValue = this.formatter.fromRaw(rawValue, this.model); this.$el.append($("", { tabIndex: -1, href: rawValue, title: this.title || formattedValue, target: this.target }).text(formattedValue)); this.delegateEvents(); return this; }

And then replace the line:

          href: rawValue,

with whatever you want the URL to be.

Personally I would prefer if Backgrid used a lot of smaller functions so that overriding stuff like this was easier. Imagine if there was a method on Cell:

getHref: function(rawValue) { return rawValue; }

and Backgrid used it instead of the rawValue you directly:

this.$el.append($("<a>", {
      tabIndex: -1,
      href: this.getUrl(rawValue),

Then instead of having to copy/paste BackGrid code in to your code, you could just overwrite getUrl:

Backgrid.Cell.extend({ getUrl: function(rawValue) { var myCalculatedUrl = doSomethingWith(rawValue); return myCalculatedUrl; })

... but it doesn't, so (as far as I can tell) you're stuck with the copy/paste.

Reply to this email directly or view it on GitHubhttps://github.com/wyuenho/backgrid/issues/242#issuecomment-36462139.

kim2014 commented 10 years ago

Should I have this render function in the model js. Sorry where I should place this render function Thanks Kim

Sent from my iPhone

On Mar 2, 2014, at 11:02 AM, "Jimmy Yuen Ho Wong" notifications@github.com<mailto:notifications@github.com> wrote:

Also, if you really would like to, here's a much simpler solution:

render: function () { this.super.render.apply(this, arguments): this.$('a').attr({'href': 'whateveruwant'}); return this; };

Reply to this email directly or view it on GitHubhttps://github.com/wyuenho/backgrid/issues/242#issuecomment-36462772.

machineghost commented 10 years ago

Exactly; however, I'd use @wyuenho's version instead of mine since his is a lot simpler.

kim2014 commented 10 years ago

Sorry for sending the email again.

I didn’t work. I added the subclass of UriCell to my model as so below and in my views I created the UriCell using subclass but got the error message:

Note: I had backbonejs’ version 1.1.0.

Uncaught TypeError: Object # has no method '_ensureElement' in backbone.js:990 Thanks Jp

n This is view createIfMapAccountGrid: function () { this.model.fetch({reset: true}); var gridContainer = this.$el;

        var columns1 = [{
            name: "id", // The key of the model attribute
            label: "ORDER", // The name to display in the header
            editable: false, // By default every cell in a column is editable, but *ID* shouldn't be

            // Backgrid.Extension.SelectRowCell lets you select individual rows
            cell: "select-row",

            // Backgrid.Extension.SelectAllHeaderCell lets you select all the row on a page
            headerCell: "select-all"
        },  {

            name: "publisherId", // The key of the model attribute
            label: "Publisher Id", // The name to display in the header
            editable: false, // By default every cell in a column is editable, but *ID* shouldn't be
            cell:  App.Cells.CustomUriCell({
              orderSeparator: ''
            })
        }, {

n Model

(function () { 'use strict'; // Name space window.App = { Models: {}, Collections: {}, Views: {}, Cells: {}

};

App.Cells.CustomUriCell = Backgrid.UriCell.extend({ render: function () { this.super.render.apply(this, arguments); this.$("a").attr({href: "TestingURL"}); return this; } });

           // IfMapAccount model has `userId` and `publiserId` attributes.
 App.Models.IfMapAccount = Backbone.Model.extend({
    idAttribute: "userId",
    // Default attributes for the device
    defaults: {
         userId: '',
         publisherId: '',
         passwordHash: ''

    },

From: Jeremy [mailto:notifications@github.com] Sent: Monday, March 03, 2014 8:16 AM To: wyuenho/backgrid Cc: Joanne Pham Subject: Re: [backgrid] No Way (That I Can Tell) To Change Target Attribute on UriCells (#242)

Exactly; however, I'd use @wyuenhohttps://github.com/wyuenho's version instead of mine since his is a lot simpler.

— Reply to this email directly or view it on GitHubhttps://github.com/wyuenho/backgrid/issues/242#issuecomment-36526016.

kim2014 commented 10 years ago

I replace the line: cell: App.Cells.CustomUriCell({ orderSeparator: '' })

To cell: App.Cells.CustomUriCell and Got the error message: Uncaught TypeError: Cannot read property 'render' of undefined And error is highlighted on ( this.super.render.apply(this, arguments);)

SubClass of UriCell

App.Cells.CustomUriCell = Backgrid.UriCell.extend({ render: function () { this.super.render.apply(this, arguments); this.$("a").attr({href: "TestingURL"}); return this; } }); Thanks Jp

From: Joanne Pham Sent: Monday, March 03, 2014 10:23 AM To: 'wyuenho/backgrid'; wyuenho/backgrid Subject: RE: [backgrid] No Way (That I Can Tell) To Change Target Attribute on UriCells (#242)

Sorry for sending the email again.

I didn’t work. I added the subclass of UriCell to my model as so below and in my views I created the UriCell using subclass but got the error message:

Note: I had backbonejs’ version 1.1.0.

Uncaught TypeError: Object # has no method '_ensureElement' in backbone.js:990 Thanks Jp

n This is view createIfMapAccountGrid: function () { this.model.fetch({reset: true}); var gridContainer = this.$el;

        var columns1 = [{
            name: "id", // The key of the model attribute
            label: "ORDER", // The name to display in the header
            editable: false, // By default every cell in a column is editable, but *ID* shouldn't be

            // Backgrid.Extension.SelectRowCell lets you select individual rows
            cell: "select-row",

            // Backgrid.Extension.SelectAllHeaderCell lets you select all the row on a page
            headerCell: "select-all"
        },  {

            name: "publisherId", // The key of the model attribute
            label: "Publisher Id", // The name to display in the header
            editable: false, // By default every cell in a column is editable, but *ID* shouldn't be
            cell:  App.Cells.CustomUriCell({
              orderSeparator: ''
            })
        }, {

n Model

(function () { 'use strict'; // Name space window.App = { Models: {}, Collections: {}, Views: {}, Cells: {}

};

App.Cells.CustomUriCell = Backgrid.UriCell.extend({ render: function () { this.super.render.apply(this, arguments); this.$("a").attr({href: "TestingURL"}); return this; } });

           // IfMapAccount model has `userId` and `publiserId` attributes.
 App.Models.IfMapAccount = Backbone.Model.extend({
    idAttribute: "userId",
    // Default attributes for the device
    defaults: {
         userId: '',
         publisherId: '',
         passwordHash: ''

    },

From: Jeremy [mailto:notifications@github.com] Sent: Monday, March 03, 2014 8:16 AM To: wyuenho/backgrid Cc: Joanne Pham Subject: Re: [backgrid] No Way (That I Can Tell) To Change Target Attribute on UriCells (#242)

Exactly; however, I'd use @wyuenhohttps://github.com/wyuenho's version instead of mine since his is a lot simpler.

— Reply to this email directly or view it on GitHubhttps://github.com/wyuenho/backgrid/issues/242#issuecomment-36526016.

wyuenho commented 10 years ago

I just answered your ticket. Please stop cross-posting all over the internet.