search

jennschiffer / timbles.js

a very simple jQuery plugin for tables, made by the person who literally did not invent tables
MIT License
38 stars 4 forks source link

Replace headerRow class with headerCell/columnHeader #35

Closed edelooff closed 8 years ago

edelooff commented 8 years ago

Over the weekend I've been working on implementing multi-row table heads (#5) and been making some pretty good progress. However, one of the things I'm running into is that with multi-row headers, the semantics of the headerRow become a little vague.

Currently, we set headerRow for the first row in <thead>, but then also only support one row there, meaning it adds no significant value.

I propose we change the behavior a bit such that we add a (configurable) columnHeader class to every active (clickable sorting) column header. This should probably not attach to any <th> elemens with a class no-sort, nor to colspanned cells in the case of multi-row / grouped headers.

Thoughts?

jennschiffer commented 8 years ago

I think that multi-row headers would be a good feature for a plugin add-on for timbles, not particularly for "core" timbles to be honest. When I first implemented multi-row headers for the NBA, that part of the configuration added a lot more complexity in terms of understanding what the the code did and how to do it, and when I first built timbles I always imagined separate modules to work with it.

That being said, I do think it's a good idea to make the names of things as semantically sound for future module add-ons. I think that columnHeader shouldn't just be restricted to sortable column headers, perhaps activeHeader would be a better class name.

jennschiffer commented 8 years ago

i knew we had this conversation before lol https://github.com/jennschiffer/timbles.js/issues/5#issuecomment-138661381

edelooff commented 8 years ago

Yeah it's an oldie, but I ran into it again and figured I'd have a stab at it again. I think I have a pretty good solution (limited to init only), but that's for the other issue at some point when it's done.

On the topic of class naming, I think a name specific to sorting makes sense. Possible future timbles features or plugins may add other behaviors to header cells (for instance spreadsheet like autofilters), making activeHeader too generic.

Another consideration, given that we're not acting on a newly created bit of document (like with the navigation button block) is prefixing. Adding a timbles- prefix ensures that the chances of unfortunate collisions are almost nil.

As such, I propose the sort-on-click header cells to get the class name timbles-sort-header (and of course, added to the data.classes object with a name like sortHeader, allowing users to change it to whatever meets their specific needs).

Before making this change (which I'd be happy to do), there is the consideration for backward compatibility. I don't suspect this change will catch people in a bad way, but removing the headerRow class from the header row does introduce backward incompatibility. In this case I'm not concerned about it (nor do I have any idea how many users timbles has), but it's something to be considered at least.