PR for 3887 - move click from col header to sort element

[edgar-koster]

This PR contains the changes to move the click from the column title element to the sort element. It adds a new variable for the sort header and adjust the styling it the default style.

[olifolkerd]

Hey @edgar-koster

Thanks for your PR.

I would not be looking to add a change that affects the default header sort functionality for all users, as this would alter the functionality of every user to just fit your usage case and a lot of users like it the way it currently is.

I would however love to add this as a feature that could be enabled if a user wants it, just as there is a headerSort option to enable sorting in the header, there could be a headerSortElement option that lets you choose to make either the full header or just the sort element clickable.

If you would be happy to make the changes then i would love to include this in the library.

I understand you are planning to make several PRs soon, could you please bare in mind with your future PR's that i am very happy to add additional options of the table but that i am very unlikely to accept a change to the default behaviour of the library that will affect every user unless there is a very strong case for it.

As this is your first time contributing to Tabulator, could i ask a couple of things of you:

• If you are submitting a PR to fix an outstanding issue, please link to the issue in the PR, simply sticking a hash in front of the issue number will do it, eg #3887
• If you are submitting a PR to handle a users feature request, please start an initial discussion on that request to gather a bit of feedback on the feature from the wider community, just to give a window for feedback

Cheers

Oli :)

[edgar-koster]

Hello Oli.

Noted. It's better to keep things as close to the current situation as possible so not to let things fall over.

I can make a change. I looked into the headerSortElement. As I see it, correct me if I'm wrong, this has three different options for displaying the icon itself. I don't think it's a forth value for this option but an option call something like headerSortIconClickable which would default to false, remain the as is situation, and set to true, change make the icon clickable. But what would a nice name be?

[olifolkerd]

Hey @edgar-koster

Ahh yes, sorry for the confusion there, i forgot that was already an option, i didn't mean to overload the existing option, maybe something like headerSortClickMode or headerSortClickElement with an option of "icon" or "header" that defaults to header, then it would be self explanatory how it functions, and allow for other customizations of where to click later if they come up.

how does that sound?

Cheers

Oli :)

[edgar-koster]

Perfect!

[edgar-koster]

I made a change. I introduced headerSortClickElement. When you don't have it set, it will be set to header. Depending on the value of this setting, either the column or the icon will be used for the event listener. Since there can only be two values, I revert to the default if not equal to icon. I also added a helper css class tabulator-col-sorter-element (like sometime there is a disabled option). With this, it's possible to also adjust the visual interaction.

[olifolkerd]

Hey @edgar-koster

Looks great,

Just a couple of points, could you change the logic that is looking for !== 'icon' to be explicitly equal to the option that you want it to be, that way if we extend this feature in future we will not need to rewrite that line.

Also i notice that you apply the class to different elements at different points in the code, would it be possible to restructure this so that the logic of where to apply the class all resides in the same place, making it easier for someone coming into this system to know what it is doing. would a switch statement work well for this?

Cheers

Oli :)

[edgar-koster]

I changed from 'if' to a 'switch' to assign the css class. This also removes the !== I had in there. I looked where the warnings are triggered for if there is a non supported option value. I thought it was an additional parameter for the registerTableOption and that function would do it, but I didn't see it there. So right now I didn't add warning for if it is not header or icon

[olifolkerd]

Hey @edgar-koster

I have merged this into the 5.4 branch and will include it in that release towards the end of the month.

Cheers

Oli :)