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

#6233 - Listbox: Preserve option groups while filtering. #6275

Merged

Conversation

avramz
Copy link
Contributor

@avramz avramz commented Aug 22, 2024

fix: Listbox: Preserve option groups while filtering.
chore: Add unit tests

###Defect Fixes
fixes #6233

Copy link

vercel bot commented Aug 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
primevue ⬜️ Ignored (Inspect) Visit Preview Oct 1, 2024 7:50pm
primevue-v3 ⬜️ Ignored (Inspect) Visit Preview Oct 1, 2024 7:50pm

@tugcekucukoglu tugcekucukoglu added the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Aug 23, 2024
const groupChildren = this.getOptionGroupChildren(group);
const filteredItems = groupChildren.filter((item) => filteredOptions.includes(item));

if (filteredItems.length > 0) filtered.push({ ...group, [typeof this.optionGroupChildren === 'string' ? this.optionGroupChildren : 'items']: [...filteredItems] });
Copy link
Member

Choose a reason for hiding this comment

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

What about typeof this.optionGroupChildren is not 'string'? It seems it returns 'items'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

items is a fallback - this method is the same as in MultiSelect -> https://github.com/primefaces/primevue/blob/master/packages/primevue/src/multiselect/MultiSelect.vue#L1012
and Select -> https://github.com/primefaces/primevue/blob/master/packages/primevue/src/select/Select.vue#L926

They both deal with the same data structure and I didn't want to reimplement the logic for consistency.
If the filtering logic for Listbox still needs to be changed I suggest that Select/MultiSelect is updated too, as they deal with the same data structures. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

As I said before, it is not correct to return a string here. We are open to new solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with optimized visibleOptions rendering logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tugcekucukoglu any feedback?

@tugcekucukoglu tugcekucukoglu removed the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Nov 13, 2024
@tugcekucukoglu tugcekucukoglu merged commit 6695e7a into primefaces:master Nov 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Listbox: OptionGroup disappear with filter
2 participants