-
Notifications
You must be signed in to change notification settings - Fork 591
feat: Add fuzzy search to SearchableSelect #3182
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: Add fuzzy search to SearchableSelect #3182
Conversation
…differences Co-authored-by: yurishkuro <[email protected]>
|
@copilot ou mentioned several times that SearchableSelect "uses Select build-in" for different features. Is that a bad thing? Can we eliminate custom NameSelect from the codebase and just use the simpler SearchableSelect? Are there any downsides of doing that? |
Using Ant Design's built-in Select is not a bad thing at all - it's actually preferred for most form use cases. Can we eliminate NameSelector? Technically yes, but there are tradeoffs: What NameSelector provides that would need redesign:
Current usage:
Migration path if desired:
The main downside of eliminating NameSelector is losing the distinctive header-style appearance. If that visual distinction isn't important, consolidating to SearchableSelect with virtualization enabled would simplify the codebase. Would you like me to explore a proof-of-concept migration? |
|
@copilot yes, let's do a proof of concept. Considering where NameSelector is used I don't think fuzzy matching is that important, but just in case - why can't it also be incorporated into SearchableSelect? |
… migration Co-authored-by: yurishkuro <[email protected]>
Created proof-of-concept in commit e1d96cf. Added to SearchableSelect:
Usage: // With fuzzy matching and virtualization
<SearchableSelect options={services} fuzzy virtual />What would need CSS work to fully replace NameSelector:
Fuzzy matching can absolutely be incorporated - it's just a matter of using a custom Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot well the code looks ok but for proof-of-concept I need it to actually replace NameSelector somewhere. Please do it in packages/jaeger-ui/src/components/TracePage/TraceStatistics/... |
…s proof-of-concept Co-authored-by: yurishkuro <[email protected]>
Done in commit d980b07. Replaced all 3 Migration pattern: // Before
<NameSelector label="Group By" options={opts} value={val} setValue={setVal} required />
// After
<label className="TraceStatisticsHeader--label">
<span className="TraceStatisticsHeader--labelText">Group By:</span>
<SearchableSelect value={val} onChange={setVal} popupMatchSelectWidth={false}>
{opts.map(opt => <Select.Option key={opt} value={opt}>{opt}</Select.Option>)}
</SearchableSelect>
</label>All 49 TraceStatistics tests pass. The main change is moving the label prefix to a separate styled Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Do not make any changes, just answer this question - where did you find that Antd Select supports virtualization? I am looking at its SelectProps type and there is no |
Signed-off-by: Yuri Shkuro <[email protected]>
You're right to question this. The Looking at the type chain:
The prop is passed through to React.createElement(_rcSelect.default, { virtual: virtual, ... })It defaults to Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3182 +/- ##
=======================================
Coverage 97.92% 97.93%
=======================================
Files 260 260
Lines 8162 8169 +7
Branches 2151 2154 +3
=======================================
+ Hits 7993 8000 +7
Misses 169 169 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lector to SearchableSelect (#3185) Completes the NameSelector → SearchableSelect migration started in #3182. Removes the now-unused NameSelector component. ## Changes - **DeepDependencies/Header**: Replace `NameSelector` with `SearchableSelect` for Service and Operation dropdowns - **QualityMetrics/Header**: Replace `NameSelector` with `SearchableSelect` for Service dropdown - **Delete NameSelector**: Remove `NameSelector.tsx`, `NameSelector.css`, `NameSelector.test.js` ## Migration Pattern Follows the same approach as TraceStatisticsHeader in #3182: ```tsx // Before (NameSelector) <NameSelector label="Service" placeholder="Select a service…" value={service || null} setValue={setService} required options={services || []} /> // After (SearchableSelect) <label className="DdgHeader--label"> <span className="DdgHeader--labelText">Service:</span> <SearchableSelect value={service || null} onChange={setService} placeholder="Select a service…" status={!service ? 'error' : undefined} fuzzy > {(services || []).map(opt => ( <Select.Option key={opt} value={opt}>{opt}</Select.Option> ))} </SearchableSelect> </label> ``` Key differences: - Label prefix in separate `<span>` with custom styling - `onChange` instead of `setValue` - `allowClear`/`onClear` for optional fields - `status="error"` for required field validation > [!WARNING] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### I tried to connect to the following addresses, but was blocked by firewall rules: > > - `iojs.org` > - Triggering command: `/usr/bin/curl curl -q --fail --compressed -L -s REDACTED -o -` (dns block) > > If you need me to access, download, or install something from one of these locations, you can either: > > - Configure [Actions setup steps](https://gh.io/copilot/actions-setup-steps) to set up my environment, which run before the firewall is enabled > - Add the appropriate URLs or hosts to the custom allowlist in this repository's [Copilot coding agent settings](https://github.com/jaegertracing/jaeger-ui/settings/copilot/coding_agent) (admins only) > > </details> <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > in the PR 3182 we migrated TransStatisticsHeader from NameSelector to SearchableSelect. I now want to complete the migration and do the same for DeepDependencies/Header and QualityMetrics/Header components. In the same PR let's delete NameSelector component and its related files. </details> <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: yurishkuro <[email protected]>
SearchableSelectandNameSelectorcomponentsSummary
Original Task
Added JSDoc comments to both components explaining their purpose and when to use each one.
Proof-of-Concept Migration (per reviewer request)
Replaced all 3 NameSelector usages in TraceStatisticsHeader with SearchableSelect:
Before (NameSelector):
After (SearchableSelect):
Key Changes
<span>with custom stylingonChangeinstead of customsetValueallowClearandonClearfor clearable selectsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.