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

a column named label #349

Closed
frague59 opened this issue Jul 4, 2016 · 8 comments · Fixed by mozilla/addons-server#4430 or drummonds/bene#50
Closed

a column named label #349

frague59 opened this issue Jul 4, 2016 · 8 comments · Fixed by mozilla/addons-server#4430 or drummonds/bene#50

Comments

@frague59
Copy link

frague59 commented Jul 4, 2016

Hi,
I've a column based on a model attribute named 'label', but this CSS class exists in bootstrap !

class ExternalApplicationTable(tables.Table):
    """
    Table for :class:`intrapubs.models.ExternalApplication` items
    """
    icon_preview = tables.TemplateColumn(template_name="tables/icon_preview.html", verbose_name=_("Icon preview"), orderable=False)
    label = tables.Column()
    url = tables.URLColumn(orderable=False)
    description = tables.Column(orderable=False)
    contact_name = tables.Column(orderable=False)
    phone_internal = tables.Column(orderable=False)
    phone_external = tables.Column(orderable=False)

    class Meta:
        attrs = META_ATTRS
        row_attrs = {
            'data-id': lambda record: record.pk
        }

When the table in rendered, I can see a weird rendering : a label css class is added to the th and th cells, making display failing : bootstrap.css defines it's own label css class, which is confusing...

I've tried to update the Colums attrs through :

    label = tables.Column(attrs={'th': {'class': 'text'}, 'td': {'class': 'text'}})

but the class label is still displayed. How can I change this ?

Thanks !

@frague59
Copy link
Author

frague59 commented Jul 4, 2016

I've found a way to change this :

_label = tables.TemplateColumn(template_code="{{ record.label }}", verbose_name=_("label"))

Its a work-arround, but it works...

@jieter
Copy link
Owner

jieter commented Jul 4, 2016

By default, columns get their name as css-class, this is expected behaviour.

I think you could change your example to _label = tables.Column(accessor='label', verbose_name=_('label'))

If I were to design this from scratch, I probably would not add this without a prefix as default behaviour, but it is what we have now. We might change it at some point to try to remove the element of suprise.

@jieter jieter closed this as completed Jul 4, 2016
@ianfp
Copy link

ianfp commented Jul 22, 2016

I was also unpleasantly surprised by this, and while the workaround above does work, it feels pretty kludgy. I looked for some way to override the default class attribute, but found nothing -- I could only append additional classes.

@jieter jieter reopened this Jul 22, 2016
@graup
Copy link
Contributor

graup commented Jul 22, 2016

I was also irritated by this at first. When there are cases when I want to style a certain column, it will usually be based on the kind of data, not the field name. So I will rather have a reusable .cell-type-number class in my CSS than a specific .number_of_foo.

We could think about removing these unprefixed classes in a 2.0 as a breaking change, but I'm not sure if that's worth it. Even if, there needs to be an easy migration path without changing every single column in every table.
Designed properly, the default behavior should be to add no classes, and only add classes based on Mixins or options.

@jieter
Copy link
Owner

jieter commented Jul 23, 2016

Reopened, we should be able to have a nice upgrade path:

  • Have some way to override how the column name is added to the css classes, for example allowing prefixing.
  • Implement the current behaviour using the new method.
  • Eventually disable adding the column name as default.
  • Add this change as changed behaviour to the changelog
  • Add a FAQ item how to restore original behaviour.

Anybody able/willing to implement this and open a PR?

@graup
Copy link
Contributor

graup commented Jul 23, 2016

For reference, the offending code is here: https://github.com/bradleyayers/django-tables2/blob/03cb7c5f6f1c98801bb7129c9a9e3d596c1a5b12/django_tables2/columns/base.py#L305-L307

@jieter what method of override would you suggest? Could Column have a get_class_name attribute, with a default of return self.name?

I could look into implementing this some time in August.

@jieter
Copy link
Owner

jieter commented Aug 9, 2016

@graup any progress?

@graup
Copy link
Contributor

graup commented Aug 10, 2016

@jieter Not yet. I might find some time next week or the week after that.

graup added a commit to graup/django-tables2 that referenced this issue Aug 15, 2016
graup added a commit to graup/django-tables2 that referenced this issue Aug 15, 2016
@jieter jieter closed this as completed in 1e73711 Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment