Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
28 changes: 25 additions & 3 deletions packages/web-components/src/radio-group/radio-group.base.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { attr, FASTElement, Observable, observable } from '@microsoft/fast-element';
import { attr, FASTElement, Observable, observable, Updates } from '@microsoft/fast-element';
import type { Radio } from '../radio/radio.js';
import { isRadio } from '../radio/radio.options.js';
import { RadioGroupOrientation } from './radio-group.options.js';

/**
Expand Down Expand Up @@ -218,14 +217,37 @@ export class BaseRadioGroup extends FASTElement {
@observable
slottedRadios!: Radio[];

private static isUpgradedRadio(element: Element): element is Radio {
return element.localName.endsWith('-radio') && '$fastController' in element;
}
Comment on lines +220 to +222
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

radio.options.ts already provides an isRadio predicate function that handles the tagname checking, and the $fastController check can be done in-place.


private getRadioDescendants(): Radio[] {
return [...this.querySelectorAll('*')].filter(BaseRadioGroup.isUpgradedRadio);
}

/**
* Updates the radios collection when the slotted radios change.
*
* @param prev - the previous slotted radios
* @param next - the current slotted radios
*/
slottedRadiosChanged(prev: Radio[] | undefined, next: Radio[]): void {
this.radios = [...this.querySelectorAll('*')].filter(x => isRadio(x)) as Radio[];
Updates.enqueue(() => {
const elements = [...this.querySelectorAll('*')];
this.radios = this.getRadioDescendants();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why get elements first and then get radio descendants separately? They both end up querying for all elements.


for (const tagName of new Set(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a weird pattern since Set provides iterator methods

elements
.filter(x => x.localName.endsWith('-radio') && !BaseRadioGroup.isUpgradedRadio(x))
.map(x => x.localName),
)) {
customElements.whenDefined(tagName).then(() => {
if (this.isConnected) {
this.radios = this.getRadioDescendants();
}
});
}
Comment on lines +240 to +249
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There's potential for a lot of repetitive and unnecessary DOM querying going on here

});
}

/**
Expand Down
79 changes: 79 additions & 0 deletions packages/web-components/src/radio-group/radio-group.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import { tagName as RadioTagName } from '../radio/radio.options.js';
import type { RadioGroup } from './radio-group.js';
import { tagName } from './radio-group.options.js';

const sourceBaseUrl = `/@fs${new URL('../', import.meta.url).pathname}`;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd rather we didn't rely on internal vite-specific constructs, or try to work outside of the bounds of the environment. I see custom pathing like this as a fragile workaround for something that can probably be handled in a better and more purposeful way. For instance, the new test can likely be its own test entrypoint.


test.describe('RadioGroup', () => {
test.use({
tagName,
Expand Down Expand Up @@ -250,6 +252,83 @@ test.describe('RadioGroup', () => {
await expect(radios.nth(2)).toHaveJSProperty('checked', false);
});

test('should preserve checked state when radios upgrade after the group', async ({ page }) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This test needs to be written using fastPage, but may need to be its own test fixture entrypoint. Doing it this way may allow us to reuse this for other components that have similar ancestor/descendant patterns.

await page.goto('/');

const result = await page.evaluate(async sourceBaseUrl => {
const id = Date.now().toString(36);
const groupTagName = `upgrade-${id}-radio-group`;
const radioTagName = `upgrade-${id}-radio`;

const importModule = (path: string): Promise<Record<string, unknown>> => import(path);
const [
radioGroupModule,
radioGroupTemplateModule,
radioGroupStylesModule,
radioModule,
radioTemplateModule,
radioStylesModule,
] = await Promise.all([
importModule(`${sourceBaseUrl}radio-group/radio-group.ts`),
importModule(`${sourceBaseUrl}radio-group/radio-group.template.ts`),
importModule(`${sourceBaseUrl}radio-group/radio-group.styles.ts`),
importModule(`${sourceBaseUrl}radio/radio.ts`),
importModule(`${sourceBaseUrl}radio/radio.template.ts`),
importModule(`${sourceBaseUrl}radio/radio.styles.ts`),
]);

const RadioGroup = radioGroupModule.RadioGroup as {
compose(options: Record<string, unknown>): { define(registry: CustomElementRegistry): void };
};
const Radio = radioModule.Radio as {
compose(options: Record<string, unknown>): { define(registry: CustomElementRegistry): void };
};

document.body.innerHTML = /* html */ `
<${groupTagName} value="bar">
<${radioTagName} value="foo"></${radioTagName}>
<${radioTagName} value="bar"></${radioTagName}>
<${radioTagName} value="baz"></${radioTagName}>
</${groupTagName}>
`;

RadioGroup.compose({
name: groupTagName,
template: radioGroupTemplateModule.template,
styles: radioGroupStylesModule.styles,
}).define(customElements);

const group = document.querySelector(groupTagName);
if (!group) {
throw new Error('Expected radio group to exist.');
}

customElements.upgrade(group);

Radio.compose({
name: radioTagName,
template: radioTemplateModule.template,
styles: radioStylesModule.styles,
}).define(customElements);

await customElements.whenDefined(groupTagName);
await customElements.whenDefined(radioTagName);
await new Promise(requestAnimationFrame);

const checkedRadio = document.querySelector(`${radioTagName}[value="bar"]`) as HTMLElement & {
checked: boolean;
};

return {
checked: checkedRadio.checked,
hasOwnChecked: Object.prototype.hasOwnProperty.call(checkedRadio, 'checked'),
};
}, sourceBaseUrl);

expect(result.checked).toBe(true);
expect(result.hasOwnChecked).toBe(false);
});

test('radio should remain checked after it is set to disabled and uncheck when a new radio is checked', async ({
fastPage,
page,
Expand Down