Svelte: unrevert `RepoPopover` #62744

camdencheek · 2024-05-16T19:09:13Z

This re-applies #61989 after it was reverted.

More detail inline, but in short:

It reverts the changes to Popover.svelte that removed the border.
We only start loading data on hover, not on mount
Various fixes in text overflow conditions
Removes the language from the popover data because it can be very expensive to calculate (another reason to pre-calculate language, but that's for another day)
Moves the data loading out of the page loader. Exports the data loading function from the component so data loading is still orchestrated by the caller. (I know this will be controversial, reasoning inline)
Adds a delay to the popover so it doesn't get in the way as your mouse moves over the page.
Uses the display name instead of the author name
Linkifies the commit message

Test plan

Storybook and lots of manual testing. Now that I'm writing this, we should probably add a playwright test too.

camdencheek · 2024-05-16T20:08:55Z

client/web-sveltekit/src/lib/Popover.svelte

@@ -109,5 +109,6 @@
        border-radius: var(--popover-border-radius);
        color: var(--body-color);
        box-shadow: var(--popover-shadow);
+        overflow: hidden;


Small addition that ensures that the contents of the popover do not overflow the popover. Notably, this ensures that the border radius gets applied to any children as well (otherwise the corners of square children will overlap the border)

camdencheek · 2024-05-16T22:59:48Z

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.gql

    name
    description
    stars
    isPrivate
-    language


I removed language because this is calculated with inventory, which is stupidly expensive and flaky on large repos. This was regularly timing out for repos like llvm.

As a more general conversation point: I do not think we should be implementing any features at this point that do not scale to large customers.

camdencheek · 2024-05-16T23:07:27Z

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte

+    export async function fetchRepoPopoverData(client: GraphQLClient, repoName: string): Promise<RepoPopoverFragment> {
+        const response = await client.query(RepoPopoverQuery, { repoName })
+        if (!response.data?.repository || response.error) {
+            throw new Error(`Failed to fetch repo info: ${response.error}`)
+        }
+        return response.data.repository
+    }


I know this is controversial, but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I think this strikes a good balance by colocating the definition of the data fetcher with the component that uses it, but not fetching the data on mount, which allows the caller to handle data fetching however it wants. In this case, we do not want to fetch the data on page load, and we also want to add a delay before rendering. That would not be possible if we fetched the data on mount inside the component.

but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I suspected that this would happen eventually, and was hoping that when that time comes we know how we want to approach this.
I actually thought about this recently. I think there is kind of a forth type of data: on demand data that is unrelated (or only loosely related) to the current URL. For this kind of data it doesn't make sense to use the data loader. We will never preload the data and it's likely that this data (components) is needed (used) on multiple pages.
I arrived at a similar solution: These components would define a "loader" that handles the data loading for them and they would get passed an instance of the loader. This way we can still easily test the component and we are separating concerns. This was with more complex data loading in mind, e.g. infinity scroll.
In a case like this it seems fine to just have a function returning the results and pass those to the component.

So yeah, I think this approach is perfectly fine and we should use similar patterns for similar components.

Side note: The whole "only fetch data in data loader" principle was never meant to be absolute. It should help us understand what kind of data we are working with and determine appropriate exceptions to this rule.

This all makes sense to me! Thanks for adding some color 🙂

camdencheek · 2024-05-16T23:09:22Z

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte

+    $: commit = data.commit
+    $: author = commit?.author
+    $: avatar = author?.person
+    $: codeHostKind = data.externalServices.nodes[0].kind
 </script>

 <div class="root">


In the component itself, I made a number of DOM and CSS simplifications, most of which did not affect rendering, a few of which fixed rendering for weird edge cases like empty description or long commit message.

camdencheek · 2024-05-16T23:10:57Z

client/web-sveltekit/src/lib/utils/promises.ts

+// Returns a promise that is guaranteed to take at least `delayMillis` milliseconds to resolve.
+// If the wrapped promise resolves before then, the returned promise will wait until `delayMillis`
+// has elapsed before resolving.
+export async function delay<T>(promise: Promise<T>, delayMillis: number): Promise<T> {
+    const [awaited] = await Promise.all([promise, new Promise(resolve => setTimeout(resolve, delayMillis))])
+    return awaited
+}


Small utility to delay resolution of a promise. We may want to add this as a general option on the Popover component, but I wanted to see how it feels first.

wip minimize divs and simplify layout/css wip fetching data not working hacky solution to the fetching problem design tweaks css tweaks popover stays visible when cursor moves toward or hovers over design tweaks and performance improvement fetch topics, not tags add useDefaultBorder option to Popover; optimize RepoPopover layout/style Popover content should control the border, not the popover itself lint and make changes from review use badges fetch repo popover data from RepoRev component remove interpunct between commit message and abbreviatedOID since it is not in the design spec remove unused imports lint remove comment revert popover changes simplify repo popover component remove duplicate border radius simplify and link to commit fix data loading behavior do not display empty popover add comment add more comments add delay drop divider, fix alignmet fix small fix spacing and colors delete redundant row nowrap appease linter

fkling

Thank you!

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.gql

fkling · 2024-05-17T06:45:10Z

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte

+    export async function fetchRepoPopoverData(client: GraphQLClient, repoName: string): Promise<RepoPopoverFragment> {
+        const response = await client.query(RepoPopoverQuery, { repoName })
+        if (!response.data?.repository || response.error) {
+            throw new Error(`Failed to fetch repo info: ${response.error}`)
+        }
+        return response.data.repository
+    }


but it was really pretty rough to thread this from page data, through every intermediate component, and finally into this component.

I suspected that this would happen eventually, and was hoping that when that time comes we know how we want to approach this.
I actually thought about this recently. I think there is kind of a forth type of data: on demand data that is unrelated (or only loosely related) to the current URL. For this kind of data it doesn't make sense to use the data loader. We will never preload the data and it's likely that this data (components) is needed (used) on multiple pages.
I arrived at a similar solution: These components would define a "loader" that handles the data loading for them and they would get passed an instance of the loader. This way we can still easily test the component and we are separating concerns. This was with more complex data loading in mind, e.g. infinity scroll.
In a case like this it seems fine to just have a function returning the results and pass those to the component.

So yeah, I think this approach is perfectly fine and we should use similar patterns for similar components.

Side note: The whole "only fetch data in data loader" principle was never meant to be absolute. It should help us understand what kind of data we are working with and determine appropriate exceptions to this rule.

client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte

cla-bot bot added the cla-signed label May 16, 2024

camdencheek force-pushed the cc/jhh/instantiate-repo-popover branch from b5660d3 to c9faa79 Compare May 16, 2024 19:10

camdencheek commented May 16, 2024

View reviewed changes

camdencheek force-pushed the cc/jhh/instantiate-repo-popover branch from 9df75e0 to b8d0282 Compare May 16, 2024 23:24

camdencheek changed the title ~~Cc/jhh/instantiate repo popover~~ Svelte: unrevert RepoPopover May 16, 2024

camdencheek added 2 commits May 16, 2024 19:55

use displayName instead of just name

aa5fd89

remove unneeded abbreviatedOID

1901b03

camdencheek marked this pull request as ready for review May 16, 2024 23:58

fkling approved these changes May 17, 2024

View reviewed changes

camdencheek added 2 commits May 21, 2024 09:10

PR feedback

bcf5f78

format

e05184b

camdencheek enabled auto-merge (squash) May 21, 2024 13:12

camdencheek merged commit 899145f into main May 21, 2024
7 checks passed

camdencheek deleted the cc/jhh/instantiate-repo-popover branch May 21, 2024 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Svelte: unrevert `RepoPopover` #62744

Svelte: unrevert `RepoPopover` #62744

camdencheek commented May 16, 2024 •

edited

camdencheek May 16, 2024 •

edited

camdencheek May 16, 2024 •

edited

camdencheek May 16, 2024

fkling May 17, 2024

camdencheek May 21, 2024

camdencheek May 16, 2024

camdencheek May 16, 2024

fkling left a comment

fkling May 17, 2024

Svelte: unrevert RepoPopover #62744

Svelte: unrevert RepoPopover #62744

Conversation

camdencheek commented May 16, 2024 • edited

Test plan

camdencheek May 16, 2024 • edited

Choose a reason for hiding this comment

camdencheek May 16, 2024 • edited

Choose a reason for hiding this comment

camdencheek May 16, 2024

Choose a reason for hiding this comment

fkling May 17, 2024

Choose a reason for hiding this comment

camdencheek May 21, 2024

Choose a reason for hiding this comment

camdencheek May 16, 2024

Choose a reason for hiding this comment

camdencheek May 16, 2024

Choose a reason for hiding this comment

fkling left a comment

Choose a reason for hiding this comment

fkling May 17, 2024

Choose a reason for hiding this comment

Svelte: unrevert `RepoPopover` #62744

Svelte: unrevert `RepoPopover` #62744

camdencheek commented May 16, 2024 •

edited

camdencheek May 16, 2024 •

edited

camdencheek May 16, 2024 •

edited