Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

search ui: add copy path button to search results#42721

Merged
limitedmage merged 2 commits into
mainfrom
jp/copypathinresults
Oct 10, 2022
Merged

search ui: add copy path button to search results#42721
limitedmage merged 2 commits into
mainfrom
jp/copypathinresults

Conversation

@limitedmage

@limitedmage limitedmage commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Stacked on #42631

Show a "copy path" button next to paths in path, content and symbol results. The button is only shown when hovering or focusing the result.

image

image

image

Test plan

Manual and visual tests

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot Bot added the cla-signed label Oct 7, 2022
@limitedmage limitedmage requested review from a team and novoselrok October 7, 2022 21:52
@sourcegraph-bot

sourcegraph-bot commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 49551e9...591bcb1.

Notify File(s)
@fkling client/search-ui/src/components/CopyPathAction.module.scss
client/search-ui/src/components/CopyPathAction.tsx
client/search-ui/src/components/FileContentSearchResult.tsx
client/search-ui/src/components/FilePathSearchResult.tsx
client/search-ui/src/components/RepoFileLink.tsx
client/search-ui/src/components/SearchResult.module.scss
client/search-ui/src/components/SymbolSearchResult.tsx
client/search-ui/src/components/snapshots/RepoFileLink.test.tsx.snap
client/search-ui/src/components/index.ts
client/search-ui/src/results/StreamingSearchResultsList.tsx

@limitedmage limitedmage force-pushed the jp/copypathinresults branch 2 times, most recently from 500b42d to 56efd97 Compare October 7, 2022 22:51

@rvantonder rvantonder left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yay! I actually spiked this and have some context. I abandoned things after I realized it would be best to factor out and share the CopyPathAction component first 😄

Show a "copy path" button next to paths in path, content and symbol results.

Side thought: I think including repo paths for repo results is useful and probably easy to include as well?

Comment on lines 15 to 17

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: how about just an opaque value : string type to represent the copied value? Since we may pass in a repoPath from the RepoRevisionContainer...

We can align that possibility with the component name by renaming CopyPathAction -> CopyAction or CopyPathAction?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would also require (in principle) making the tooltip and aria-label props, since it would now be a generic button. I think I'll keep it specifically as a "copy path" button for now, and we can look into making it more generic next time we need a copy button in the app. Since the "copy repo" action is ambiguous, I'll punt that for now as well.

@limitedmage

Copy link
Copy Markdown
Contributor Author

Side thought: I think including repo paths for repo results is useful and probably easy to include as well?

What would we copy here, the repo name without github.com? With github.com? The full URL on Sourcegraph? I don't see much of a benefit to doing this personally, and the behavior is not very predictable :/ Also the site admin can make repo names render differently in a way that may not be useful to copy.

Base automatically changed from rn/file-search-result-refactor to rn/result-container-refactor October 10, 2022 06:17
Base automatically changed from rn/result-container-refactor to main October 10, 2022 06:54
@limitedmage limitedmage merged commit 75efbc9 into main Oct 10, 2022
@limitedmage limitedmage deleted the jp/copypathinresults branch October 10, 2022 20:52
novoselrok pushed a commit that referenced this pull request Oct 14, 2022
novoselrok pushed a commit that referenced this pull request Oct 14, 2022
* Revert "search ui: add copy path button to search results (#42721)"

This reverts commit 75efbc9.

* Revert "search ui: search results components hierarchy refactor (#42483)"

This reverts commit 7c2e624.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants