Svelte: add repo popovers to repo name in header and in dynamic filters #62865
Conversation
camdencheek
force-pushed
the
cc/repo-popover-search-results
branch
from
5ed07ce
to
a22641e
Compare
| <slot name="item" {item}> | ||
| <SectionItem {item} {onFilterSelect}> | ||
| <slot name="label" slot="label" let:label let:value {label} {value} /> | ||
| </SectionItem> | ||
| </slot> |
There was a problem hiding this comment.
Extracted this into a new component to make it easier to create a slot with a default, but overridable, value.
| type SectionItem, | ||
| type SectionItemData, |
There was a problem hiding this comment.
Renamed because I wanted a component named SectionItem now, so it makes sense to call this SectionItemData instead.
| <svelte:fragment slot="item" let:item> | ||
| <Popover showOnHover let:registerTrigger placement="right-start"> | ||
| <div use:registerTrigger> | ||
| <SectionItem {item}> | ||
| <svelte:fragment slot="label" let:label> | ||
| <CodeHostIcon disableTooltip repository={label} /> | ||
| <span>{displayRepoName(label)}</span> | ||
| </svelte:fragment> | ||
| </SectionItem> | ||
| </div> | ||
| <svelte:fragment slot="content"> | ||
| {#await delay(fetchRepoPopoverData(getGraphQLClient(), item.label), 200) then data} | ||
| <RepoPopover {data} withHeader /> | ||
| {:catch error} | ||
| <Alert size="slim" variant="danger">{error}</Alert> | ||
| {/await} | ||
| </svelte:fragment> | ||
| </Popover> | ||
| </svelte:fragment> |
There was a problem hiding this comment.
We want to wrap the whole item in the popover, not just the repo name, because otherwise the popover placement varies depending on the length of the repo name.
|
The one on the repo header on the repo pages needs to be offset a little on Y-axis (down a little). |
| @@ -35,23 +31,11 @@ | |||
| <ul> | |||
| {#each limitedItems as item} | |||
There was a problem hiding this comment.
By the way, do we need to specify a key properly in this loop here?
There was a problem hiding this comment.
I don't think so since the array is immutable. But we should probably be explicit about it
| @@ -60,7 +64,16 @@ | |||
|
|
|||
| <GlobalHeaderPortal> | |||
| <nav aria-label="repository"> | |||
| <h1><a href="/{repoName}">{displayRepoName}</a></h1> | |||
| <Popover showOnHover placement="bottom-start" let:registerTrigger> | |||
| <h1 use:registerTrigger><a href="/{repoName}">{displayRepoName}</a></h1> | |||
There was a problem hiding this comment.
I would add a hover state here, it's been a case that people don't understand that this H1 element is clickable, since we are going to have a popover element to this hover state also makes sense here IMO
There was a problem hiding this comment.
Agree, but I think we should save this for the planned header redesign
client/web-sveltekit/src/lib/search/dynamicFilters/Section.svelte
Outdated
Show resolved
Hide resolved
camdencheek
force-pushed
the
cc/repo-popover-search-results
branch
from
00ef510
to
02696ac
Compare
| <SectionItem {item} {onFilterSelect}> | ||
| <slot name="label" slot="label" let:label let:value {label} {value} /> | ||
| </SectionItem> | ||
| {#if $$slots.label} |
There was a problem hiding this comment.
Can this if statement live within SectionItems tags here? Or this would break the default slot as well?
There was a problem hiding this comment.
Unfortunately, it seems there's an edge case where slot rendering cannot be conditional within a component. So I had to copy the fully component in the conditional. Another thing that gets much nicer in svelte 5
This adds the
RepoPopoverhover info to the dynamic filters in the search sidebar (replacing the full repo name tooltip) and to the global header.Test plan
screenshot-2024-05-22_14-30-16.mp4