Skip to content

Commit

Permalink
Combobox registerOption reversion to address Angular bug (#1949)
Browse files Browse the repository at this point in the history
# Pull Request

## 🀨 Rationale

[AzDO bug 'Nimble combo box | Issue in showing list
options'](https://ni.visualstudio.com/DevCentral/_workitems/edit/2687002)

## πŸ‘©β€πŸ’» Implementation

Simple reversion of problem code. I've also created a [tech debt
item](#1948) to track the work needed
to ultimately re-align the `Combobox` and `Select` implementations.

## πŸ§ͺ Testing

Just removing the related test.

## βœ… Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
  • Loading branch information
atmgrifter00 authored Mar 20, 2024
1 parent 55ba7aa commit 83f7fd9
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 51 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Remove ListOptionOwner from Combobox to address issue found in Angular",
"packageName": "@ni/nimble-components",
"email": "26874831+atmgrifter00@users.noreply.github.com",
"dependentChangeType": "patch"
}
25 changes: 2 additions & 23 deletions packages/nimble-components/src/combobox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,10 @@ import { iconExclamationMarkTag } from '../icons/exclamation-mark';

import { styles } from './styles';
import type { ErrorPattern } from '../patterns/error/types';
import type {
DropdownPattern,
ListOptionOwner
} from '../patterns/dropdown/types';
import type { DropdownPattern } from '../patterns/dropdown/types';
import { DropdownAppearance } from '../patterns/dropdown/types';
import type { AnchoredRegion } from '../anchored-region';
import { template } from './template';
import type { ListOption } from '../list-option';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -37,7 +33,7 @@ declare global {
*/
export class Combobox
extends FoundationCombobox
implements DropdownPattern, ErrorPattern, ListOptionOwner {
implements DropdownPattern, ErrorPattern {
@attr
public appearance: DropdownAppearance = DropdownAppearance.underline;

Expand Down Expand Up @@ -211,23 +207,6 @@ export class Combobox
return returnValue;
}

/**
* @internal
*/
public registerOption(option: ListOption): void {
if (this.options.includes(option)) {
return;
}

// Adding an option to the end, ultimately, isn't the correct
// thing to do, as this will mean the option's index in the options,
// at least temporarily, does not match the DOM order. However, it
// is expected that a successive run of `slottedOptionsChanged` will
// correct this order issue. See 'https://github.com/ni/nimble/issues/1915'
// for more info.
this.options.push(option);
}

protected override focusAndScrollOptionIntoView(): void {
if (this.open) {
super.focusAndScrollOptionIntoView();
Expand Down
29 changes: 1 addition & 28 deletions packages/nimble-components/src/combobox/tests/combobox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
waitAnimationFrame
} from '../../utilities/tests/component';
import { checkFullyInViewport } from '../../utilities/tests/intersection-observer';
import { ListOption, listOptionTag } from '../../list-option';
import { listOptionTag } from '../../list-option';

async function setup(
position?: string,
Expand Down Expand Up @@ -127,33 +127,6 @@ describe('Combobox', () => {
await disconnect();
});

it('option added directly to DOM synchronously registers with Combobox', async () => {
const { element, connect, disconnect } = await setup();
await connect();
element.selectedIndex = 0;
await waitForUpdatesAsync();
const newOption = new ListOption('foo', 'foo');
const registerOptionSpy = spyOn(
element,
'registerOption'
).and.callThrough();
registerOptionSpy.calls.reset();
element.insertBefore(newOption, element.options[0]!);

expect(registerOptionSpy.calls.count()).toBe(1);
expect(element.options).toContain(newOption);

// While the option is registered synchronously as shown above,
// properties like selectedIndex will only be correct asynchronously
// See https://github.com/ni/nimble/issues/1915
expect(element.selectedIndex).toBe(0);
await waitForUpdatesAsync();
// This assertion shows that after 'slottedOptionsChanged' runs, the
// 'selectedIndex' state has been corrected to expected DOM order.
expect(element.selectedIndex).toBe(1);
await disconnect();
});

const ariaTestData: {
attrName: string,
propSetter: (x: Combobox, value: string) => void
Expand Down

0 comments on commit 83f7fd9

Please sign in to comment.