(feat) add repo metadata to repo root page by erzhtor · Pull Request #50607 · sourcegraph/sourcegraph-public-snapshot

erzhtor · 2023-04-13T17:45:36Z

Part of https://github.com/sourcegraph/pr-faqs/issues/96. Follow-up https://github.com/sourcegraph/sourcegraph/pull/50567.

This PR adds repo metadata to repo root page

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
Open repo root page

Screenshots

Description	Screenshot
Root page
Root page with `isFork=true`

App preview:

Web

Check out the client app preview documentation to learn more.

sourcegraph-bot · 2023-04-13T17:47:37Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6595172...fe7eb42.

Notify	File(s)
@fkling	client/branded/src/search-ui/components/RepoMetadata.module.scss client/branded/src/search-ui/components/RepoMetadata.story.tsx client/branded/src/search-ui/components/RepoMetadata.tsx client/branded/src/search-ui/components/RepoSearchResult.tsx client/branded/src/search-ui/components/SearchResult.module.scss client/branded/src/search-ui/components/index.ts
@limitedmage	client/branded/src/search-ui/components/RepoMetadata.module.scss client/branded/src/search-ui/components/RepoMetadata.story.tsx client/branded/src/search-ui/components/RepoMetadata.tsx client/branded/src/search-ui/components/RepoSearchResult.tsx client/branded/src/search-ui/components/SearchResult.module.scss client/branded/src/search-ui/components/index.ts

sourcegraph-bot · 2023-04-13T17:48:26Z

Codenotify: Notifying subscribers in OWNERS files for diff 6595172...fe7eb42.

No notifications.

sg-e2e-regression-test-bob · 2023-04-13T17:53:13Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.05% (+6.65 kb)	0.06% (+6.65 kb)	0.13% (+1) 🔺

Look at the Statoscope report for a full comparison between the commits fe7eb42 and 6595172 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

camdencheek

Two small comments:

Is that color actually used elsewhere in the app? It's a little bit jarring 😄 Also, because it stands out so much, I think it emphasizes those badges more than is probably desirable.
How does this handle many pieces of metadata?

Leaving the code review to someone more qualified 🙂

kopancek · 2023-04-14T06:07:09Z

Two small comments:

Is that color actually used elsewhere in the app? It's a little bit jarring 😄 Also, because it stands out so much, I think it emphasizes those badges more than is probably desirable.

How does this handle many pieces of metadata?

Leaving the code review to someone more qualified 🙂

Agreed that we probably need a proper design for this feature, but this looks like a good pre-alpha step forward.

I'm not sure using the badges is the right approach. Also, I have no idea how to distinguish metadata related to ownership vs something else. Having just one layer of key-value pairs of strings might not cut it.

kopancek · 2023-04-14T06:11:06Z

You can instead use one <Badge with 2 children and then style just the background of the one you need to change the colour of, so something like:

Suggested change



<Badge small={small} variant="info" className={classNames({ [styles.repoMetadataKey]: value })}>

{key}

</Badge>

<Badge small={small} variant="secondary" className={styles.repoMetadataValue}>

{value}

</Badge>



<Badge small={small} variant="secondary" className={styles.repoMetadataKey} key={`${key}:${value}`}>

{key}

{value}

</Badge>

Looks cleaner and IMO is a better resulting HTML structure. You also do not need to touch border radius at all this way.

I don't think this is better than current version, as it will require resetting padding of inner child and setting bg color of one of them. I think using badge with variants (info, secondary) is more explicit.

I'm also not a fan of this markup, but mostly because I think this is confusing for accessibility. How is a screen reader user meant to know that one value is the key and the next one is the value? I think we need to rethink this; eg. adding some hidden text to add key/value labels.

Good point, I've updated to use description list HTML tags, as well as added aria attributes. Would appreciate the feedback.

Awesome solution, love this!

kopancek · 2023-04-14T06:14:12Z

Why don't we let the RepoMetadata deal with the structure of keyValuePairs like before? What is the benefit of doing Object.entries here vs in the RepoMetadata component?

I changed it for easier handling of the GQL response of keyValuePairs, which is {key: 'key', value: 'value'}[] format. So with the previous structure,

we had to convert it to props format like Object.fromEntries(repo.keyValuePairs.map(({ key, value }) => [key, value]))}

and then inside RepoMetadata convert back to tuples as Object.entries(keyValuePairs)

But let me know if you think previous type/version is better, we can revert back to it.

0xnmn · 2023-04-17T06:45:52Z

I suggest to rename this at graphql API level in a separate PR to something more meaningful like metadata.

keyValuePairs explains nothing.

Good point. Currently, it is being used by src cli and probably by customers. I will clarify how safe to rename this and, based on that follow-up.

…eePage test refactor(TreePage.tsx): extract repository metadata into a separate component and wrap it in a div to fix alignment issue refactor(PageHeader.tsx): remove unused rootClassName prop and its usage in the component

…eyValuePairs array to Repository type object

erzhtor requested review from a team, camdencheek, limitedmage and rrhyne April 13, 2023 17:45

erzhtor self-assigned this Apr 13, 2023

cla-bot Bot added the cla-signed label Apr 13, 2023

camdencheek approved these changes Apr 13, 2023

View reviewed changes

kopancek reviewed Apr 14, 2023

View reviewed changes

erzhtor requested a review from kopancek April 14, 2023 06:46

0xnmn reviewed Apr 17, 2023

View reviewed changes

Comment thread client/wildcard/src/components/PageHeader/PageHeader.tsx Outdated

erzhtor force-pushed the erzhtor/add-repo-metadata-to-repo-root-page branch from ea4ab01 to f972c65 Compare April 17, 2023 16:19

limitedmage approved these changes Apr 17, 2023

View reviewed changes

Comment thread client/wildcard/src/components/PageHeader/PageHeader.tsx Outdated

erzhtor added 6 commits April 18, 2023 13:24

feat: add repo metadata to repo root page

e57b99c

fixup! feat: add repo metadata to repo root page

89488d2

Add metadata memoization and ordering

81179de

feat(client): add RepoMetadata component to branded build target

e119a6e

refactor RepoMetadata component and add storybook story

96643eb

erzhtor force-pushed the erzhtor/add-repo-metadata-to-repo-root-page branch from 198e572 to 2e4709f Compare April 18, 2023 07:24

refactor(commit-page.test.ts, graphQlResponseHelpers.ts): add empty k…

fe7eb42

…eyValuePairs array to Repository type object

erzhtor merged commit 2967c96 into main Apr 18, 2023

erzhtor deleted the erzhtor/add-repo-metadata-to-repo-root-page branch April 18, 2023 09:12

erzhtor mentioned this pull request Apr 18, 2023

Epic: repository metadata #43961

Open

6 tasks

-                <span className="d-flex align-items-center justify-content-center" key={`${key}:${value}`}>
-                    <Badge small={small} variant="info" className={classNames({ [styles.repoMetadataKey]: value })}>
-                        {key}
-                    </Badge>
-                    <Badge small={small} variant="secondary" className={styles.repoMetadataValue}>
-                        {value}
-                    </Badge>
-                </span>
+                <Badge small={small} variant="secondary" className={styles.repoMetadataKey} key={`${key}:${value}`}>
+                    <span className="key">{key}<span>
+                    <span className="value">{value}</span>
+                </Badge>

Conversation

erzhtor commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Screenshots

App preview:

Uh oh!

sourcegraph-bot commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sourcegraph-bot commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sg-e2e-regression-test-bob commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report 📦

Uh oh!

camdencheek left a comment

Choose a reason for hiding this comment

Uh oh!

kopancek commented Apr 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

erzhtor commented Apr 13, 2023 •

edited

Loading

sourcegraph-bot commented Apr 13, 2023 •

edited

Loading

sourcegraph-bot commented Apr 13, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Apr 13, 2023 •

edited

Loading