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

feat(table): update select all on addition or removal of rows #632

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -438,11 +438,26 @@ <h2>Data table with sortable columns</h2>
</gux-table>

<h2>Object table with rows selection</h2>
<ul>
<li>
Use the <strong>setChecked</strong> method on
<strong>gux-all-row-select</strong> to programatically manage the selected
state.
</li>
<li>
Use the <strong>setIndeterminate</strong> method to set the
indeterminate/unknown state on <strong>gux-all-row-select</strong>. Remember
to set it to false when the checkbox is checked or unchecked as it overrides
these states.
</li>
</ul>
<gux-button id="selectAll">Select all</gux-button>
<gux-button id="deselecAll">Deselect all</gux-button>
<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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be easier to just query select the gux-all-row-select element instead of adding an id.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

<th data-column-name="first-name">First name</th>
<th data-column-name="last-name">Last name</th>
<th data-column-name="age" data-cell-numeric>Age</th>
Expand Down Expand Up @@ -970,7 +985,21 @@ <h2>Data table with fixed columns</h2>
}


})
});

const selectAll = document.getElementById('selectAll');
const deselectAll = document.getElementById('deselecAll');
const allRowSelect = document.getElementById('allRowSelect');

selectAll.addEventListener('click', () => {
allRowSelect.setIndeterminate(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

allRowSelect.setChecked(true);
});

deselectAll.addEventListener('click', () => {
allRowSelect.setIndeterminate(false);
allRowSelect.setChecked(false);
});
})()"
>
.not-used {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export class GuxAllRowSelect {
onCheck(event: CustomEvent): void {
event.stopPropagation();
this.selected = this.inputElement.checked;
this.internalallrowselectchange.emit(this.inputElement.checked);
this.internalallrowselectchange.emit(this.selected);
}

@Method()
Expand All @@ -49,6 +49,12 @@ export class GuxAllRowSelect {
this.inputElement.indeterminate = indeterminate;
}

@Method()
async setChecked(checked: boolean): Promise<void> {
this.selected = checked;
this.internalallrowselectchange.emit(this.selected);
}

async componentWillLoad(): Promise<void> {
this.i18n = await buildI18nForComponent(
this.root,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,22 @@

## Methods

### `setChecked(checked: boolean) => Promise<void>`



#### Parameters

| Name | Type | Description |
| --------- | --------- | ----------- |
| `checked` | `boolean` | |

#### Returns

Type: `Promise<void>`



### `setIndeterminate(indeterminate?: boolean) => Promise<void>`


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,38 @@ describe('gux-all-row-select', () => {

expect(page.rootInstance).toBeInstanceOf(GuxAllRowSelect);
});

it('should handle the setIndeterminate method', async () => {
const html = '<gux-all-row-select></gux-all-row-select>';
const page = await newSpecPage({ components, html, language });

const component = page.rootInstance;
const inputElement = page.root.shadowRoot.querySelector('input');

expect(inputElement.indeterminate).toBe(undefined);

await component.setIndeterminate(true);
expect(inputElement.indeterminate).toBe(true);

await component.setIndeterminate(false);
expect(inputElement.indeterminate).toBe(false);
});

it('should handle the setChecked method', async () => {
const html = '<gux-all-row-select></gux-all-row-select>';
const page = await newSpecPage({ components, html, language });

const component = page.rootInstance;
const inputElement = page.root.shadowRoot.querySelector('input');

expect(inputElement.checked).toBe(false);

await component.setChecked(true);
await page.waitForChanges();
expect(inputElement.checked).toBe(true);

await component.setChecked(false);
await page.waitForChanges();
expect(inputElement.checked).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ export class GuxTable {
root: HTMLElement;

private resizeObserver: ResizeObserver;
private slotObserver: MutationObserver = new MutationObserver(() =>
forceUpdate(this)
);
private slotObserver: MutationObserver = new MutationObserver(() => {
this.prepareSelectableRows();
forceUpdate(this);
});
private i18n: GetI18nValue;
private columnResizeState: GuxTableColumnResizeState | null;
private tableId: string = randomHTMLId('gux-table');
Expand Down Expand Up @@ -267,11 +268,9 @@ export class GuxTable {

// Handle a change in state of the select all checkbox
private handleSelectAllRows(selected: boolean): void {
const selectAllCheckbox = this.selectAllCheckbox;
const rowCheckboxes = this.rowCheckboxes;

rowCheckboxes.forEach(rowBox => {
selectAllCheckbox.selected = selected;
if (!rowBox.disabled) {
rowBox.selected = selected;
this.updateRowSelection(rowBox);
Expand Down