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

Svelte [RepoPopover]: Instantiate the RepoPopover result across the web-app#61989

Merged
jasonhawkharris merged 24 commits into
mainfrom
jhh/instantiate-repo-popover
May 14, 2024
Merged

Svelte [RepoPopover]: Instantiate the RepoPopover result across the web-app#61989
jasonhawkharris merged 24 commits into
mainfrom
jhh/instantiate-repo-popover

Conversation

@jasonhawkharris

@jasonhawkharris jasonhawkharris commented Apr 17, 2024

Copy link
Copy Markdown
Contributor

This commit includes optimizations and improvements for the RepoPopover, in addition to instantiating the component on the Repo search results page. Not much has changed on the design from the implementation PR. Here is a video to show some live interaction:

Screen.Recording.2024-04-23.at.1.17.27.PM.mov
  1. @fkling I'd like to get your thoughts on how I've pulled the data in for this. @camdencheek mentioned you might have thoughts on this approach.

Test plan

manual, visual test, storybooks, unit tests.

@cla-bot cla-bot Bot added the cla-signed label Apr 17, 2024
@jasonhawkharris jasonhawkharris changed the title Jhh/instantiate repo popover Svelte [RepoPopover]: Instantiate the RepoPopover result across the web-app Apr 18, 2024
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch 5 times, most recently from 0bcf10d to 7a698d3 Compare April 23, 2024 18:13
@jasonhawkharris jasonhawkharris marked this pull request as ready for review April 23, 2024 18:22
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch from 7a698d3 to 285af09 Compare April 23, 2024 18:23
@sourcegraph-bot

sourcegraph-bot commented Apr 23, 2024

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread dev/update-embeddings-list/tsconfig.json Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/routes/search/+page.svelte Outdated
Comment thread client/web-sveltekit/src/lib/Popover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/Popover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/routes/search/+page.svelte Outdated
Comment thread client/web-sveltekit/src/routes/search/RepoRev.svelte Outdated
Comment thread dev/update-embeddings-list/tsconfig.json Outdated
Comment thread client/web-sveltekit/src/routes/+layout.ts Outdated
Comment thread client/web-sveltekit/src/routes/+layout.ts Outdated
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch from 285af09 to b7107a5 Compare April 24, 2024 14:59
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch 2 times, most recently from be8bc3b to f8a65af Compare May 2, 2024 17:32
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch 3 times, most recently from 5c1c919 to 8397f0a Compare May 6, 2024 20:10
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch from 8397f0a to 089b553 Compare May 13, 2024 14:09
@camdencheek

Copy link
Copy Markdown
Member

Can we increase the hover delay slightly? It's pretty annoying to have the hovercard show up just when moving the mouse across the screen.

screenshot-2024-05-13_15-23-56.mp4

Comment thread client/web-sveltekit/src/lib/repo/RepoPopover/RepoPopover.svelte Outdated
Comment thread client/web-sveltekit/src/routes/search/+page.ts Outdated
Comment thread client/web-sveltekit/src/routes/search/RepoRev.svelte Outdated

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

screenshot-2024-05-13_15-28-34@2x

Comment thread client/web-sveltekit/src/routes/search/RepoRev.svelte Outdated
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch from 089b553 to f7784f8 Compare May 14, 2024 14:51
@jasonhawkharris jasonhawkharris force-pushed the jhh/instantiate-repo-popover branch from f7784f8 to 3ad6a9d Compare May 14, 2024 14:53
@jasonhawkharris jasonhawkharris enabled auto-merge (squash) May 14, 2024 15:14
@jasonhawkharris jasonhawkharris merged commit 0d8003f into main May 14, 2024
@jasonhawkharris jasonhawkharris deleted the jhh/instantiate-repo-popover branch May 14, 2024 15:17
vovakulikov added a commit that referenced this pull request May 15, 2024
vovakulikov added a commit that referenced this pull request May 15, 2024
…ss the web-app (#62684)

Revert "Svelte [RepoPopover]: Instantiate the RepoPopover result across the web-app (#61989)"

This reverts commit 0d8003f.
camdencheek added a commit that referenced this pull request May 21, 2024

This re-applies #61989 after it was reverted. In addition to reapplying the change:

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

4 participants