search ui: separate SymbolSearchResult component#42500
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff adf71b0...70a56a1.
|
limitedmage
left a comment
There was a problem hiding this comment.
Looks good! Left some questions and suggestions.
| @@ -0,0 +1,32 @@ | |||
| .symbols-override { | |||
There was a problem hiding this comment.
Not sure why it's happening, but the Percy visual tests show regressions with line number alignment on symbol results.
There was a problem hiding this comment.
There was a rouge SCSS style applied, I removed it, looks OK now.
| index={index} | ||
| telemetryService={telemetryService} | ||
| result={result} | ||
| onSelect={() => logSearchResultClicked(index, 'fileMatch')} |
There was a problem hiding this comment.
Can we update this log to be specific to symbol results? (Same with the other. We should probably just log result.type everywhere.)
| <PrefetchableFile | ||
| isPrefetchEnabled={prefetchFileEnabled} | ||
| prefetch={prefetchFile} | ||
| filePath={result.path} | ||
| revision={getRevision(result.branches, result.commit)} | ||
| repoName={result.repository} | ||
| className={resultContainerStyles.resultContainer} | ||
| as="li" | ||
| > |
There was a problem hiding this comment.
This wrapper seems to be the same for content, path and symbol matches. An alternative here is to make an extra internal switch statement and have this same wrapper for all three "file-level" results. Another option is to move the prefetchable wrapper into the search result components so that they are always prefetchable.
| @@ -14,13 +14,7 @@ import { MatchGroup, MatchItem } from '@sourcegraph/shared/src/components/rankin | |||
| import { ZoektRanking } from '@sourcegraph/shared/src/components/ranking/ZoektRanking' | |||
There was a problem hiding this comment.
Consider renaming this file/component to FileContentSearchResults?
There was a problem hiding this comment.
I'll do that in a separate PR, I'm planning on refactoring the FileSearchResult/FileMatchChildren components as well.
| return ( | ||
| <Component | ||
| className={classNames('test-search-result', styles.resultContainer, className)} | ||
| className={classNames('test-search-result', className, styles.resultContainer)} |
There was a problem hiding this comment.
Why reorder here? We usually want the passed-in styles to be able to override the default styles.
|
I'll merge this into the base PR to avoid a large PR stack. I'll work on refactoring the FileSearchResult component in a separate PR. You can still comment on this PR if needed. |
Part of #42159
Summary
FileMatchChildrenandCodeExcerptcomponents (everything is copy-pasted into thecodeLinkNavigation.tsfile, no changes)FileMatchChildrento a standaloneSymbolSearchResultcomponentTest plan
App preview:
Check out the client app preview documentation to learn more.