Tradeshift / tradeshift-ui

The Tradeshift UI Library & Framework
https://ui.tradeshift.com
Other
33 stars 45 forks source link

[Table] Mention the “All Pages” button in the documentation. #438

Closed ghost closed 6 years ago

ghost commented 7 years ago

Bug report

Table onselectall not triggering when handler defined either in table.selectable or table.onselectall.

Tradeshift UI version affected

v9.3.2

Expected Behavior

On ticking the checkbox to select all rows in the table, onselectall handler is triggered.

Actual Behavior

On ticking the checkbox to select all rows in the table onselectall handler is not triggered.

Steps to reproduce

Lookup the ProductCategorization in v4apps. It's main table is generated in this file: https://github.com/Tradeshift/Apps/blob/master/src/main/v4apps/apps/ProductCategorization/src/browser/components/ProductTableUI/ProductTableUI.js

wiredearp commented 7 years ago

@ciprianpratiats The onselectall callback is not supposed to fire when the user clicks the checkbox. Clicking the checkbox simply selects the visible rows (the current page) and not "all' the rows. The onselectall event only fires fire when the user explicitly chooses All Pages in the menu. It is possible that we will change the behavior of the checkbox at some point (cc @bomouridsen), but unless I have misread the issue report, the Table exhibits the expected behavior. It is possible that we should make this more clear in the documentation.

ghost commented 7 years ago

On ui.tradeshift.com it sais:

If you are loading the table data incrementally (so you manage paging yourself), you might like to know when all or none is selected via the menu. The Table supports two additional callbacks that can be assigned via selectable().

table.selectable(onselect, onselectall, onunselectall);

You can also assign these methods directly.

table.onselect = function(selected, unselected) {};
table.onselectall = function() { console.log('All'); };
table.onunselectall = function() { console.log('None'); };

There is NO mention of the "All Pages" button.

wiredearp commented 7 years ago

Righ. I’ll reopen the issue and change the description to: Mention the “All Pages” button in the documentation. The callback is supposed to indicate that the user wants to select all products in the database, not only the products that is currently mounted in the Table, and this is only possible via the All Pages button. If the Table only has a subset of the available data mounted, it should never indicate that "all" has been selected or unselected just because the subset might be, but I can see why you would think so after reading the docs.

ghost commented 7 years ago

I think that the functionality is not fully thought through. In my case, the pagination is handled by backend. If the table only displays a subset, by clicking the checkbox, it should trigger onselectall event directly without any menu. Unselectall event works fine by the way.

wiredearp commented 7 years ago

The fundamental UI pattern is admittedly absurd, but this is how it is supposed to work: Clicking the checkbox only selects the current page, not all pages. The only way to select all pages, and without selecting them manually one by one, is the press the All Pages button.

So the checkbox doesn't do what you think it does (or rather, it is not supposed to do what you are trying to make it do).

We have considered changing the behavior of the checkbox, but that is how it works for now. You can see the intended behavior on http://ui.tradeshift.com/#components/table/demo.html. If you click Build Everything, we load all the data into the Table (so that it is not a "serverside" Table). Click the checkbox to see the current page selected. Now navigate to page 2 via the Pager in the bottom and witness that it is not selected. To make everything selected (in the Table or in your database), you have to choose All Pages via the popup menu. Only the button, and not the checkbox, should trigger the onselectall callback 😏