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

(Alt.) Refactor BoundColumn attributes to allow override of class names. #349 #370

Merged
merged 3 commits into from
Aug 22, 2016

Conversation

graup
Copy link
Contributor

@graup graup commented Aug 19, 2016

Related issue: #349
Original PR: #367

So now I added Table.get_column_class_names which gets the original set of class names (parsed from column attributes) as well as the bound column to replicate the old behavior but allow to override it.

This might still need a bit more documentation, I think.


return attrs

def get_td_class_name(self, td_attrs):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be get_td_class_names or just get_td_class?

@jieter
Copy link
Owner

jieter commented Aug 19, 2016

Thanks for working on this secont option, I think I like this one better, API is simpler.

Not totally sure about:

  • the name of get_column_class_names,
  • in the case of the <th>, a note about the adding addition of ordering/descending/ascending to the class might be in place.
  • while writing the previous point, I noticed we have a different but similar way in which we allow customisation of the classes for ordering. Why not use something like that to tackle this problem? (Not very well thought through yet).
  • As for documenting it, I think the get_colum_class_names docstr should mention it is intended to be overridden to allow customisation.
  • Some others which I commented on inline.

@graup
Copy link
Contributor Author

graup commented Aug 19, 2016

I fixed the things you mentioned and added more documentation.

I agree with your third point, it is still a bit weird to have a completely different API for this. I was also thinking about how we could get this into the attributes, but the only option I saw would be to allow passing a function (or lambda) into that, which is also a bit messy.

@graup
Copy link
Contributor Author

graup commented Aug 19, 2016

At least with this version the (eventual) removal of the name class and migration path is very clear. We just have to remove the line classes_set.add(bound_column.name) and tell people to override the function to add it back.

@jieter
Copy link
Owner

jieter commented Aug 22, 2016

Decided to merge, thanks @graup!

@jieter jieter merged commit 1e73711 into jieter:master Aug 22, 2016
@jieter jieter mentioned this pull request Jan 4, 2018
5 tasks
This was referenced Mar 8, 2018
@jieter
Copy link
Owner

jieter commented Apr 10, 2018

I just did a pre-release of 2.0.0 with version 2.0.0a0. Please help by testing and report any issues.

AppleJin512 pushed a commit to AppleJin512/django-tables2 that referenced this pull request Feb 21, 2023
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

Successfully merging this pull request may close these issues.

2 participants