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.

Sign up for GitHub

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

fix(combobox): refresh behavior of the list and position #719

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(combobox): refresh behavior of the list and position #719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
104 changes: 79 additions & 25 deletions packages/ods/src/components/combobox/src/components/ods-combobox/ods-combobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ export class OdsCombobox implements OdsFormElement {
@State() private hasNoResults: boolean = false;
@State() private isInvalid: boolean | undefined;
@State() private isOpen: boolean = false;
@State() private items: ResultElement[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it is only meant to force a refresh of the element list when slot change.
Plus you just pass its ref to this.resultElements anyway.

If we do want to update the list without closing the list, we could just use the forceUpdate method to trigger a rerender.

I wonder even if it could be better to close and the reopen the dropdown when items change so that it isi visible by the user, otherwise the contents he is currently on, is suddenly modified (then the isOpen state will trigger the refresh)


@Prop({ reflect: true }) public addNewElementLabel: string = 'Add';
@Prop({ reflect: true }) public allowMultiple: boolean = false;
Expand Down Expand Up @@ -366,6 +367,23 @@ export class OdsCombobox implements OdsFormElement {
this.closeDropdown();
}

private hasDiff(oldItems: ResultElement[], newItems: ResultElement[]): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice to move it into the controller and add unit tests

if (oldItems.length !== newItems.length) {
return true;
}

const oldValues = new Set(oldItems.map((item) => item.getAttribute('value')));

for (const newItem of newItems) {
const value = newItem.getAttribute('value');
if (!oldValues.has(value)) {
return true;
}
}

return false;
}

private onBlur(event: FocusEvent): void {
if (this.el.contains(event.relatedTarget as Node)) {
return;
Expand Down Expand Up @@ -499,26 +517,34 @@ export class OdsCombobox implements OdsFormElement {
}

private onSlotChange(event: Event): void {
this.resultElements = (event.currentTarget as HTMLSlotElement).assignedElements()
this.resultGroups = [];

const newItems = (event.currentTarget as HTMLSlotElement).assignedElements()
.reduce((res, element) => {
if (element.tagName === 'ODS-COMBOBOX-ITEM') {
res.push(element as ResultElement);
return res;
} else if (element.tagName === 'ODS-COMBOBOX-GROUP') {
this.resultGroups.push(element as ResultGroup);
res = res.concat(
(Array.from(element.children) as ResultElement[]).filter((el) => el.tagName === 'ODS-COMBOBOX-ITEM')
.map((el: ResultElement) => {
el.groupId = element.id;
return el;
}),
);
}
return res;
}, [] as ResultElement[]);

if (element.tagName === 'ODS-COMBOBOX-GROUP') {
this.resultGroups.push(element as ResultGroup);
if (this.hasDiff(this.items, newItems)) {
this.items = [...newItems];

return res.concat((Array.from(element.children || []) as ResultElement[])
.filter((el) => el.tagName === 'ODS-COMBOBOX-ITEM')
.map((el: ResultElement) => {
el.groupId = element.id;
return el;
}));
}
this.resultElements = this.items;

return res;
}, [] as ResultElement[]) ;
if (this.isOpen) {
this.filterResults(this.inputElement?.value || '');
}
}
}

private onTagKeyDown(event: KeyboardEvent): void {
Expand Down Expand Up @@ -601,24 +627,52 @@ export class OdsCombobox implements OdsFormElement {
}

private async openDropdown(): Promise<void> {
if (this.resultListElement && this.searchElement) {
if (!this.resultListElement || !this.searchElement) {
return;
}

this.resultListElement.classList.remove('ods-combobox__results--bottom', 'ods-combobox__results--top');
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be duplicated, idealy it should be done on close, and we should always close and reopen, but it's TBD as mentionned on the state comment.

this.searchElement.classList.remove('ods-combobox__search--bottom', 'ods-combobox__search--top');

this.isOpen = true;

// setTimeout to allow to render the element before calculating the position
requestAnimationFrame(async() => {
if (!this.isOpen || !this.resultListElement) {
return;
}

const { placement, y } = await getElementPosition('bottom', {
popper: this.resultListElement,
trigger: this.el,
}, {
offset: -1, // offset the border-width size as we want it merged with the trigger.
offset: -1,
});

Object.assign(this.resultListElement.style, {
left: '0',
top: `${y}px`,
});
if (!this.isOpen || !this.resultListElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

done twice (see first instruction of RaF)

return;
}

this.isOpen = true;
this.resultListElement?.scrollTo(0, 0);
this.resultListElement.classList.add(`ods-combobox__results--${placement}`);
this.searchElement.classList.add(`ods-combobox__search--${placement}`);
}
if (placement.startsWith('top')) {
Object.assign(this.resultListElement.style, {
bottom: `${this.searchHeight}px`,
left: '0',
top: 'auto',
Copy link
Contributor

Choose a reason for hiding this comment

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

needed?

});
} else {
Object.assign(this.resultListElement.style, {
bottom: 'auto',
Copy link
Contributor

Choose a reason for hiding this comment

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

needed?

left: '0',
top: `${y}px`,
});
}

const finalPlacement = placement.split('-')[0];
this.resultListElement.classList.add(`ods-combobox__results--${finalPlacement}`);
this.searchElement?.classList.add(`ods-combobox__search--${finalPlacement}`);

this.resultListElement.scrollTo(0, 0);
});
}

private async openResultList(): Promise<void> {
Expand Down Expand Up @@ -798,4 +852,4 @@ export class OdsCombobox implements OdsFormElement {
</Host>
);
}
}
}
Loading