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

(feat) add repository metadata to repo match search result items#50567

Merged
erzhtor merged 7 commits into
mainfrom
erzhtor/add-repo-metadata-to-search-results
Apr 13, 2023
Merged

(feat) add repository metadata to repo match search result items#50567
erzhtor merged 7 commits into
mainfrom
erzhtor/add-repo-metadata-to-search-results

Conversation

@erzhtor

@erzhtor erzhtor commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Part of https://github.com/sourcegraph/pr-faqs/issues/96.

This PR:

  • Adds repository metadata (aka keyValuePairs) to search repo match result items if repository-metadata feature flag is enabled

Test plan

  • sg start
  • Enable repository-metadata feature flag
  • Add some key value pairs through API (currently no UI for adding) GQL mutation addRepoKeyValuePair API
  • Search and check search results

Screenshots

Description Screenshot
Repo match image

App preview:

Check out the client app preview documentation to learn more.

@erzhtor erzhtor self-assigned this Apr 12, 2023
@cla-bot cla-bot Bot added the cla-signed label Apr 12, 2023
@erzhtor erzhtor marked this pull request as ready for review April 12, 2023 17:43
@sourcegraph-bot

sourcegraph-bot commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff c9ddb0d...68b9168.

Notify File(s)
@fkling client/branded/src/search-ui/components/RepoSearchResult.tsx
client/branded/src/search-ui/components/SearchResult.module.scss
client/shared/src/search/stream.ts
client/web/src/search/results/StreamingSearchResults.tsx
@limitedmage client/branded/src/search-ui/components/RepoSearchResult.tsx
client/branded/src/search-ui/components/SearchResult.module.scss
client/web/src/search/results/StreamingSearchResults.tsx

camdencheek
camdencheek reviewed Apr 12, 2023
View reviewed changes

@camdencheek camdencheek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Woot! Thanks for tackling this.

A couple of quick first impressions:

  1. It seems a little strange that we are adding this to all result types since they are logically associated only with the repos, not a file or a commit. I would expect these to only be displayed on repository result types.
  2. The added gap between the title line and the result card is a little visually jarring. Might be worth a design consultation here
  3. What does this look like with a null value? I don't see any screenshots with that case.

@sg-e2e-regression-test-bob

sg-e2e-regression-test-bob commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (0.00 kb) 0.01% (+1.63 kb) 0.01% (+1.63 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 68b9168 and 3f84c0f or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

@limitedmage

limitedmage commented Apr 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This is a great addition! I agree with Camden's comments here. I recommend reconsidering the design to put these key/value pairs inside the box and only for repo results.

CleanShot 2023-04-12 at 10 53 29

I also recommend showing them in the repo page (eg. https://sourcegraph.com/github.com/sourcegraph/sourcegraph), but this can be a separate PR.

@erzhtor

erzhtor commented Apr 12, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for a quick review, @camdencheek.

  1. It seems a little strange that we are adding this to all result types since they are logically associated only with the repos, not a file or a commit. I would expect these to only be displayed on repository result types.

I intentionally added to all result types except own since they all associate with repos, but we can remove and leave only for repo match too. cc @rrhyne @ryphil would appreciate your feedback.

  1. The added gap between the title line and the result card is a little visually jarring. Might be worth a design consultation here

Yep, already asked Rob Rhyne for help with designs.

  1. What does this look like with a null value? I don't see any screenshots with that case.

Good point, I missed that values are nullable, will take a look at this scenario too.

kopancek
kopancek reviewed Apr 13, 2023
View reviewed changes

@kopancek kopancek 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.

Mostly naming and styling questions/suggestions

Comment thread client/branded/src/search-ui/components/ResultContainer.tsx Outdated
Comment thread client/branded/src/search-ui/components/ResultContainer.tsx Outdated
Comment thread client/branded/src/search-ui/components/ResultContainer.module.scss Outdated
Comment thread client/branded/src/search-ui/components/ResultContainer.module.scss Outdated
Comment thread client/branded/src/search-ui/components/ResultContainer.module.scss Outdated
Comment thread client/branded/src/search-ui/components/FileContentSearchResult.tsx Outdated

@kopancek kopancek Apr 13, 2023

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.

If we call the feature repository metadata, why do we call everything that actually implements it key value pairs? Does not make sense to me to be honest.

@erzhtor erzhtor Apr 13, 2023

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.

I believe metadata is implemented as simple key-value pairs for more flexibility.

kopancek
kopancek reviewed Apr 13, 2023
View reviewed changes
Comment thread client/branded/src/search-ui/components/ResultContainer.tsx Outdated
@erzhtor erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from 3bec0e0 to 159c93c Compare April 13, 2023 08:54
@erzhtor erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from 90f4e33 to 52c6bf9 Compare April 13, 2023 09:18
sashaostrikov
sashaostrikov approved these changes Apr 13, 2023
View reviewed changes

@sashaostrikov sashaostrikov 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.

Nice! LGTM after your changes and fixes.
I guess you'll move it as Juliana proposed in the next PR?

@erzhtor erzhtor force-pushed the erzhtor/add-repo-metadata-to-search-results branch from d344949 to c0a148f Compare April 13, 2023 09:40
@erzhtor

erzhtor commented Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

Thanks, everyone, for the reviews and feedback. So after thinking a bit,

  • I followed @camdencheek suggestion on showing repository metadata only on RepoMatch results. We can add to other search result types if needed in future
    • Also added handling of null value cases (see screenshot for oss badge)
  • I've followed @limitedmage suggestion and moved badges to header section
    • I'll also follow up in a separate PR @limitedmage's suggestion to add these badges to repo page too
  • I've also followed @kopancek suggestion and reused badges with little styling from our current wildcard. Metadata key is info variant badge and value is secondary variant badge

Here is how it looks now:

image

@erzhtor erzhtor requested a review from kopancek April 13, 2023 10:15
@erzhtor erzhtor requested a review from camdencheek April 13, 2023 10:15
@erzhtor erzhtor changed the title (feat) add repository metadata "keyValuePairs" to search result items (feat) add repository metadata to repo match search result items Apr 13, 2023
@erzhtor erzhtor merged commit 4c07309 into main Apr 13, 2023
@erzhtor erzhtor deleted the erzhtor/add-repo-metadata-to-search-results branch April 13, 2023 11:52
This was referenced Apr 13, 2023
Open
Merged
@erzhtor

erzhtor commented Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

I also recommend showing them in the repo page (eg. https://sourcegraph.com/github.com/sourcegraph/sourcegraph), but this can be a separate PR.

@limitedmage Follow-up PR adding repo metadata to repo root page. I would appreciate reviews. cc @camdencheek @camdencheek

almeidapaulooliveira pushed a commit that referenced this pull request Apr 13, 2023
@erzhtor @almeidapaulooliveira
(feat) add repository metadata to repo match search result items (#50567
6d9f178 
)
cesrjimenez pushed a commit that referenced this pull request Apr 14, 2023
@erzhtor @cesrjimenez
(feat) add repository metadata to repo match search result items (#50567
322e1d1 
)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@keegancsmith keegancsmith Awaiting requested review from keegancsmith

@limitedmage limitedmage Awaiting requested review from limitedmage

@rrhyne rrhyne Awaiting requested review from rrhyne

@stefanhengl stefanhengl Awaiting requested review from stefanhengl

@camdencheek camdencheek Awaiting requested review from camdencheek

2 more reviewers

@kopancek kopancek kopancek approved these changes

@sashaostrikov sashaostrikov sashaostrikov approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@erzhtor erzhtor

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@erzhtor @sourcegraph-bot @sg-e2e-regression-test-bob @limitedmage @kopancek @camdencheek @sashaostrikov