-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat(combobox): highlight filter matches #9425
Conversation
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! 🫘
} | ||
|
||
return parts; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this is helpful, but this is what we have for the maps sdk.
const regexContainsHTML = /<[a-z/][\s\S]*>/i;
const boldKeywords = (text: string, keywords: string): string => {
const escapedKeywords = escapeRegExpString(keywords);
const splitKeywords = escapedKeywords.split(/\s/);
const pattern = new RegExp(`(${splitKeywords.join("|")})`, "gi");
return text.replace(pattern, (match) => `<strong>${match}</strong>`);
};
... later
return regexContainsHTML.test(text) ? text : boldKeywords(text, searchTerm);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to follow the same logic since Search widget would likely want the same bold experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. Will update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, could we discuss this in a follow-up issue? There are some differences in implementation and UI/UX that I think need to be considered first, such as:
- combobox filtering is immediate, search has minimum chars before showing matches
- combobox highlights the first occurrence, search highlights all matches
- combobox items do not support HTML as content, search does
There could be others, but these are the ones I noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did add the regexp char escape. 🏃🔠 Thanks for the bringing that up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
combobox filtering is immediate, search has minimum chars before showing matches
I think this could be handled when we support combobox with dynamic data
combobox highlights the first occurrence, search highlights all matches
Shouldn't combobox highlight all matches? Why does it only highlight the first occurrence?
combobox items do not support HTML as content, search does
Yes, we can ignore this one. It doesn't do regex if there is HTML involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't combobox highlight all matches? Why does it only highlight the first occurrence?
This closely matches the current filtering behavior. It returns on the first match. I also did this to minimize elements updated as the user types since we don't have virtualization alternatives and showing all matches < 3 chars seems unnecessary.
FWIW, I'm fine revisiting ☝️, but I would like design to give a pass before doing so. It would be an incremental enhancement this way vs rolling it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds good.
packages/calcite-components/src/components/combobox-item/combobox-item.tsx
Outdated
Show resolved
Hide resolved
@driskull I think I've addressed all items besides the highlighting behavior discussion. Would you mind reviewing again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good!
Related Issue: #9026
Summary
Adds highlighting to combobox for the first occurrence of an item's label that matches the filter text.
Next steps
For consistency, we can update
list
with similar functionality.