Skip to content

Commit

Permalink
fix(button-toggle-group): fix race condition between values property …
Browse files Browse the repository at this point in the history 
…and slotted buttons (#1324)

* Fix some tests descriptions

* Solve race condition between slot change and values

* Add tests for null and empty cases for values
  • Loading branch information
@YonatanKra
YonatanKra committed Jun 28, 2022
1 parent e582f2e commit 3477513
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 21 deletions.
30 changes: 20 additions & 10 deletions components/button-toggle-group/src/vwc-button-toggle-group.ts
View file
Expand Up @@ -67,7 +67,7 @@ export class VWCButtonToggleGroup extends LitElement {
}

#_items: Element[] | null = null;

#_values: (string | false | null)[] = [];
get items(): Element[] {
return this.#_items ? this.#_items : (this.#_items = [...this.children].filter(child => isValidButton(child)));
}
Expand All @@ -85,12 +85,22 @@ export class VWCButtonToggleGroup extends LitElement {
}

set values(values: (string | false | null)[]) {
this.clearSelection();
function isButtonSelectedWithoutValue(button: Element) {
return !button.hasAttribute('value') && button.hasAttribute(SELECTED_ATTRIBUTE_NAME);
}

function isButtonWithDefinedValue(button: Element) {
return button.hasAttribute('value') && values?.includes(button.getAttribute('value'));
}

if (!this.multi) {
values = [values[0]];
}
this.items.forEach((child) => {
this.toggleButtonSelectedState(child, values.includes(child.getAttribute('value')));
this.#_values = values;

this.items.forEach((button) => {
this.toggleButtonSelectedState(button,
isButtonSelectedWithoutValue(button) || isButtonWithDefinedValue(button));
});
}

Expand All @@ -107,14 +117,14 @@ export class VWCButtonToggleGroup extends LitElement {
if (this.dense && this.enlarged) {
this.enlarged = false;
}
this.updateComplete.then(() => this.#_items?.forEach(buttonElement => this.setVwcButtonSize(buttonElement)));
this.updateComplete.then(() => this.items?.forEach(buttonElement => this.setVwcButtonSize(buttonElement)));
}

if (changes.has('enlarged')) {
if (this.enlarged && this.dense) {
this.dense = false;
}
this.updateComplete.then(() => this.#_items?.forEach(buttonElement => this.setVwcButtonSize(buttonElement)));
this.updateComplete.then(() => this.items?.forEach(buttonElement => this.setVwcButtonSize(buttonElement)));
}

if (changes.has('disabled')) {
Expand All @@ -130,7 +140,7 @@ export class VWCButtonToggleGroup extends LitElement {
.filter(node => (isValidButton(node) && !this.items.includes(node)));
this.setNodesAndClickEvents(nodes);
this.#_items = null;
this.items;
this.values = [...new Set([...this.#_values, ...this.values])];
});
}

Expand All @@ -152,7 +162,7 @@ export class VWCButtonToggleGroup extends LitElement {
}

private isButtonValidForToggle(buttonElement: Element) {
return !this.required || this.values.length > 1 ||
return !this.required || this.selected.length > 1 ||
(this.required && !isButtonActive(buttonElement));
}

Expand Down Expand Up @@ -184,10 +194,10 @@ export class VWCButtonToggleGroup extends LitElement {
private toggleButtonSelectedState(buttonElement: Element, isSelected: boolean) {
if (isSelected) {
buttonElement.setAttribute('layout', 'filled');
buttonElement.setAttribute(SELECTED_ATTRIBUTE_NAME, '');
buttonElement.toggleAttribute(SELECTED_ATTRIBUTE_NAME, true);
} else {
buttonElement.removeAttribute(SELECTED_ATTRIBUTE_NAME);
buttonElement.removeAttribute('layout');
buttonElement.toggleAttribute(SELECTED_ATTRIBUTE_NAME, false);
}
}

Expand Down
77 changes: 66 additions & 11 deletions components/button-toggle-group/test/toggle-buttons-group.test.js
View file
Expand Up @@ -146,7 +146,7 @@ describe('Toggle-buttons-group', () => {
.equal(actualElement.children[1]);
});

it(`should set the elements in the array of the last button that was clicked`, function () {
it(`should set the elements in the array as the last button that was clicked`, function () {
actualElement.children[1].click();
actualElement.children[2].click();

Expand All @@ -172,7 +172,7 @@ describe('Toggle-buttons-group', () => {
);
});

it(`should return the an empty array if none is selected`, function () {
it(`should return an empty array if none is selected`, function () {
expect(actualElement.values.length)
.to
.equal(0);
Expand All @@ -189,7 +189,7 @@ describe('Toggle-buttons-group', () => {
.equal(buttonValues[1]);
});

it(`should set the value in the array of the last button that was clicked`, function () {
it(`should set the last button that was clicked in the selected array`, function () {
actualElement.children[1].click();
actualElement.children[2].click();

Expand All @@ -201,20 +201,40 @@ describe('Toggle-buttons-group', () => {
.equal(buttonValues[2]);
});

it(`should set the group's state according to set values`, function () {
it(`should set the group's state according to set values`, async function () {
const oldValues = actualElement.values;
actualElement.values = [buttonValues[0], buttonValues[2]];
const newValues = actualElement.values;
await actualElement.updateComplete;
const newValues = actualElement.selected;
expect(oldValues.length)
.to
.equal(0);
expect(newValues.length)
.to
.equal(1);
expect(newValues[0])
expect(newValues[0].getAttribute('value'))
.to
.equal(buttonValues[0]);
});

it('should return empty values array when set to null', async function() {
actualElement.multi = true;
actualElement.values = [buttonValues[0], buttonValues[2]];
await actualElement.updateComplete;
actualElement.values = null;
await actualElement.updateComplete;
expect(actualElement.values.length).to.equal(0);
expect(actualElement.selected.length).to.equal(0);
});

it('should return empty values array when set to []', async function() {
actualElement.multi = true;
actualElement.values = [buttonValues[0], buttonValues[2]];
await actualElement.updateComplete;
actualElement.values = [];
expect(actualElement.values.length).to.equal(0);
expect(actualElement.selected.length).to.equal(0);
});
});

describe(`multi`, function () {
Expand Down Expand Up @@ -307,6 +327,15 @@ describe('Toggle-buttons-group', () => {
.to
.equal('22');
});

it(`should apply the right state to a dynamically added element`, async function () {
actualElement.values = ['22'];
const element = document.createElement(VALID_BUTTON_ELEMENTS[0]);
element.setAttribute('value', '22');
actualElement.appendChild(element);
await waitForSlotChange();
expect(actualElement.selected[0].getAttribute('value')).to.equal('22');
});
});

describe(`size`, function () {
Expand Down Expand Up @@ -385,7 +414,19 @@ describe('Toggle-buttons-group', () => {
</${COMPONENT_NAME}>`));
}

it(`should not cancel the last selection on click`, async function () {
it(`should prepopulate the values according to set selected buttons`, async function () {
const [actualElement] = (generateTemplate(['multi'],
[
['selected', 'value="1"'],
[],
['selected', 'value="2"'],
['selected', 'value="3"']
]));

expect(JSON.stringify(actualElement.values)).to.equal(JSON.stringify(['1', '2', '3']));
});

it(`should leave last selection on click`, async function () {
const [actualElement] = (generateTemplate(['required', 'multi']));

await actualElement.updateComplete;
Expand All @@ -409,8 +450,22 @@ describe('Toggle-buttons-group', () => {
.equal(selectedButton);
});

it(`should not cancel the last selection when button preselected`, async function () {
const [actualElement] = addElement(generateTemplate(['required', 'multi'],
it(`should deselect an active element when clicked`, async function () {
const [actualElement] = (generateTemplate(['required', 'multi'],
[['selected'], [], [], []]));
await actualElement.updateComplete;
const selectedButton = actualElement.children[1];

selectedButton.click();
await actualElement.updateComplete;
selectedButton.click();
await actualElement.updateComplete;

expect(selectedButton.hasAttribute('selected')).to.equal(false);
});

it(`should leave the last selection when button preselected`, async function () {
const [actualElement] = (generateTemplate(['required', 'multi'],
[['selected'], [], [], []]));

await actualElement.updateComplete;
Expand All @@ -433,8 +488,8 @@ describe('Toggle-buttons-group', () => {
.equal(selectedButton);
});

it(`should not send an event if not canceling`, async function () {
const [actualElement] = addElement(generateTemplate(['required', 'multi'],
it(`should prevent event emit if unselect is not available`, async function () {
const [actualElement] = (generateTemplate(['required', 'multi', 'dense'],
[['selected'], [], [], []]));

await actualElement.updateComplete;
Expand Down

0 comments on commit 3477513

Please sign in to comment.