feat(table): update select all on addition or removal of rows #632
Conversation
|
Demo will be published at https://apps.inindca.com/common-ui-docs/genesys-webcomponents/feature/COMUI-2770_table_check_all_status |
github-actions
bot
requested review from
daragh-king-genesys and
MattCheely
| @@ -49,6 +50,26 @@ export class GuxAllRowSelect { | |||
| this.inputElement.indeterminate = indeterminate; | |||
| } | |||
|
|
|||
| @Method() | |||
| async setCheckedState(checkedState: GuxAllRowSelectState): Promise<void> { | |||
| switch (checkedState) { | |||
There was a problem hiding this comment.
Maybe it is an too "clever" and what you have is clearer but I would have
async setCheckedState(checkedState: GuxAllRowSelectState): Promise<void> {
this.inputElement.checked = checkedState === 'checked';
this.inputElement.indeterminate = checkedState === 'indeterminate';
}
| @@ -49,6 +50,26 @@ export class GuxAllRowSelect { | |||
| this.inputElement.indeterminate = indeterminate; | |||
| } | |||
|
|
|||
| @Method() | |||
| async setCheckedState(checkedState: GuxAllRowSelectState): Promise<void> { | |||
There was a problem hiding this comment.
The naming of this function might be a bit off as it sets the checked state and the indeterminate state on the internal input.
| async setCheckedState(checkedState: GuxAllRowSelectState): Promise<void> { | ||
| switch (checkedState) { | ||
| case 'checked': | ||
| this.inputElement.checked = true; |
There was a problem hiding this comment.
This is ignoring the components selected property which I think is wrong.
Should it be this.selected = true;
| @@ -49,6 +50,26 @@ export class GuxAllRowSelect { | |||
| this.inputElement.indeterminate = indeterminate; | |||
| } | |||
|
|
|||
| @Method() | |||
| async setCheckedState(checkedState: GuxAllRowSelectState): Promise<void> { | |||
There was a problem hiding this comment.
If this function is useful we should be able to use it in the updateSelectAllBoxState method of gux-table and it should simplify the code/make it easier to read.
| forceUpdate(this) | ||
| ); | ||
| private slotObserver: MutationObserver = new MutationObserver(() => { | ||
| if (this.selectAllCheckbox) { |
There was a problem hiding this comment.
I am not sure this if guard is needed. The updateSelectAllBoxState method that is eventually calls has a guard against the select all checkbox not existing already.
|
What should the behaviour be if this is set dynamically ? At the moment if I set the checked state dynamically using the function it does work for example I pass |
|
@daragh-king-genesys Keeping the setting of checked & indeterminate states separate to keep it inline with the native checkbox. If we try & update the table rows themeselves based on a possible 3 states we won't know what to do with 'indeterminate' without a lot of work. |
| <gux-table object-table selectable-rows> | ||
| <table slot="data"> | ||
| <thead> | ||
| <tr data-row-id="head"> | ||
| <th><gux-all-row-select></gux-all-row-select></th> | ||
| <th><gux-all-row-select id="allRowSelect"></gux-all-row-select></th> |
There was a problem hiding this comment.
I think it might be easier to just query select the gux-all-row-select element instead of adding an id.
There was a problem hiding this comment.
That would select all the gux-all-row-select's on the page, I just want to select that particular one as it relates to one particular table.
| const allRowSelect = document.getElementById('allRowSelect'); | ||
|
|
||
| selectAll.addEventListener('click', () => { | ||
| allRowSelect.setIndeterminate(false); |
There was a problem hiding this comment.
Would it be better to house this within the setChecked method in the shadowDOM like,
async setChecked(checked: boolean): Promise<void> {
this.selected = checked;
this.setIndeterminate(false);
this.internalallrowselectchange.emit(this.selected);
}
or
async setChecked(checked: boolean): Promise<void> {
this.selected = checked;
this.inputElement.indeterminate = false
this.internalallrowselectchange.emit(this.selected);
}
as I am not sure is it valid to call a method with a Watch decorator internally.
There was a problem hiding this comment.
Yeah I did initially look at that but decided to keep the setting of checked & indeterminate states separate to keep it inline with the native checkbox as there were a few issues.
Apparently 'indeterminate' is a presentational state, just visual, while the value of a checkbox is either checked or unchecked. I thought it best to stick with this to avoid any unforeseen issues down the road by diverging from the native checkbox.
I've an example with select/deselect all buttons using the methods on the |
Select all checkbox will update if a row is deleted from the dom.
I've also added a new method to manually set the state of gux-all-row-select.