Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Svelte: add repo popovers to repo name in header and in dynamic filters #62865

Merged
merged 6 commits into from
May 30, 2024
Merged

Svelte: add repo popovers to repo name in header and in dynamic filters #62865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f9f0c2
wip
camdencheek May 21, 2024
7e31690
wip
camdencheek May 22, 2024
f7a8bcb
add popover to repo name in header
camdencheek May 22, 2024
382eeef
re-simplify slots
camdencheek May 22, 2024
02696ac
fix slot propagation
camdencheek May 29, 2024
d6ff20a
expand repo button for nicely spaced popover
camdencheek May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 13 additions & 69 deletions client/web-sveltekit/src/lib/search/dynamicFilters/Section.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
<script lang="ts">
import { mdiClose } from '@mdi/js'

import { page } from '$app/stores'
import Icon from '$lib/Icon.svelte'
import { Button } from '$lib/wildcard'

import CountBadge from './CountBadge.svelte'
import { updateFilterInURL, type SectionItem } from './index'
import type { SectionItemData } from './index.ts'
import SectionItem from './SectionItem.svelte'

export let items: SectionItem[]
export let items: SectionItemData[]
export let title: string
export let filterPlaceholder: string = ''
export let showAll: boolean = false
Expand All @@ -35,23 +31,17 @@
<ul>
{#each limitedItems as item}
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, do we need to specify a key properly in this loop here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so since the array is immutable. But we should probably be explicit about it

<li>
<a
href={updateFilterInURL($page.url, item, item.selected).toString()}
class:selected={item.selected}
on:click={() => onFilterSelect(item.kind)}
>
<span class="label">
<slot name="label" label={item.label} value={item.value}>
{item.label}
</slot>
</span>
<CountBadge count={item.count} exhaustive={item.exhaustive} />
{#if item.selected}
<span class="close">
<Icon svgPath={mdiClose} inline />
</span>
<slot name="item" {item}>
{#if $$slots.label}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this if statement live within SectionItems tags here? Or this would break the default slot as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

<SectionItem {item} {onFilterSelect}>
<svelte:fragment slot="label" let:label let:value>
<slot name="label" {label} {value} />
</svelte:fragment>
</SectionItem>
{:else}
<SectionItem {item} {onFilterSelect} />
{/if}
</a>
</slot>
</li>
{/each}
</ul>
Expand Down Expand Up @@ -135,50 +125,4 @@
.show-more {
text-align: center;
}

a {
display: flex;
width: 100%;
align-items: center;
border: none;
text-align: left;
text-decoration: none;
border-radius: var(--border-radius);
color: inherit;
white-space: nowrap;
gap: 0.25rem;

padding: 0.25rem 0.5rem;
margin: 0;
font-weight: 400;

.label {
flex: 1;
text-overflow: ellipsis;
overflow: hidden;
color: var(--text-body);
}

&:hover {
background-color: var(--color-bg-3);

.label {
color: var(--text-title);
}
}

&.selected {
background-color: var(--primary);
color: var(--light-text);
--color: var(--light-text);

.label {
color: var(--light-text);
}
}

.close {
flex-shrink: 0;
}
}
</style>
78 changes: 78 additions & 0 deletions client/web-sveltekit/src/lib/search/dynamicFilters/SectionItem.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<script lang="ts">
import { mdiClose } from '@mdi/js'

import { page } from '$app/stores'
import Icon from '$lib/Icon.svelte'

import CountBadge from './CountBadge.svelte'
import { updateFilterInURL, type SectionItemData } from './index'

export let item: SectionItemData
export let onFilterSelect: (kind: SectionItemData['kind']) => void = () => {}
</script>

<a
href={updateFilterInURL($page.url, item, item.selected).toString()}
class:selected={item.selected}
on:click={() => onFilterSelect(item.kind)}
>
<span class="label">
<slot name="label" label={item.label} value={item.value}>
{item.label}
</slot>
</span>
<CountBadge count={item.count} exhaustive={item.exhaustive} />
{#if item.selected}
<span class="close">
<Icon svgPath={mdiClose} inline />
</span>
{/if}
</a>

<style lang="scss">
a {
display: flex;
width: 100%;
align-items: center;
border: none;
text-align: left;
text-decoration: none;
border-radius: var(--border-radius);
color: inherit;
white-space: nowrap;
gap: 0.25rem;

padding: 0.25rem 0.5rem;
margin: 0;
font-weight: 400;

.label {
flex: 1;
text-overflow: ellipsis;
overflow: hidden;
color: var(--text-body);
}

&:hover {
background-color: var(--color-bg-3);

.label {
color: var(--text-title);
}
}

&.selected {
background-color: var(--primary);
color: var(--light-text);
--color: var(--light-text);

.label {
color: var(--light-text);
}
}

.close {
flex-shrink: 0;
}
}
</style>
38 changes: 27 additions & 11 deletions client/web-sveltekit/src/lib/search/dynamicFilters/Sidebar.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,20 +32,24 @@

import { goto } from '$app/navigation'
import { page } from '$app/stores'
import { getGraphQLClient } from '$lib/graphql'
import Icon from '$lib/Icon.svelte'
import ArrowBendIcon from '$lib/icons/ArrowBend.svelte'
import LanguageIcon from '$lib/LanguageIcon.svelte'
import Popover from '$lib/Popover.svelte'
import RepoPopover, { fetchRepoPopoverData } from '$lib/repo/RepoPopover/RepoPopover.svelte'
import CodeHostIcon from '$lib/search/CodeHostIcon.svelte'
import SymbolKindIcon from '$lib/search/SymbolKindIcon.svelte'
import { displayRepoName, scanSearchQuery, type Filter } from '$lib/shared'
import { SVELTE_LOGGER, SVELTE_TELEMETRY_EVENTS } from '$lib/telemetry'
import Tooltip from '$lib/Tooltip.svelte'
import { delay } from '$lib/utils'
import { Alert } from '$lib/wildcard'
import Button from '$lib/wildcard/Button.svelte'

import HelpFooter from './HelpFooter.svelte'
import {
type URLQueryFilter,
type SectionItem,
type SectionItemData,
Comment on lines -48 to +52
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed because I wanted a component named SectionItem now, so it makes sense to call this SectionItemData instead.

staticTypeFilters,
typeFilterIcons,
groupFilters,
Expand All @@ -54,14 +58,15 @@
} from './index'
import LoadingSkeleton from './LoadingSkeleton.svelte'
import Section from './Section.svelte'
import SectionItem from './SectionItem.svelte'

export let searchQuery: string
export let streamFilters: Filter[]
export let selectedFilters: URLQueryFilter[]
export let state: 'complete' | 'error' | 'loading'

$: groupedFilters = groupFilters(streamFilters, selectedFilters)
$: typeFilters = staticTypeFilters.map((staticTypeFilter): SectionItem => {
$: typeFilters = staticTypeFilters.map((staticTypeFilter): SectionItemData => {
const selectedOrStreamFilter = groupedFilters.type.find(
typeFilter => typeFilter.label === staticTypeFilter.label
)
Expand All @@ -83,7 +88,7 @@
}
}

function handleFilterSelect(kind: SectionItem['kind']): void {
function handleFilterSelect(kind: SectionItemData['kind']): void {
SVELTE_LOGGER.log(SVELTE_TELEMETRY_EVENTS.SelectSearchFilter, { kind }, { kind })
}

Expand Down Expand Up @@ -119,13 +124,24 @@
filterPlaceholder="Filter repositories"
onFilterSelect={handleFilterSelect}
>
<svelte:fragment slot="label" let:label>
<Tooltip tooltip={label} placement="right">
<span>
<CodeHostIcon disableTooltip repository={label} />
<span>{displayRepoName(label)}</span>
</span>
</Tooltip>
<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>
Comment on lines +127 to 145
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

</Section>
<Section
Expand Down
4 changes: 2 additions & 2 deletions client/web-sveltekit/src/lib/search/dynamicFilters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { Filter } from '@sourcegraph/shared/src/search/stream'
import { parseExtendedSearchURL } from '..'
import { SearchCachePolicy, setCachePolicyInURL } from '../state'

export type SectionItem = Omit<Filter, 'count'> & {
export type SectionItemData = Omit<Filter, 'count'> & {
count?: Filter['count']
selected: boolean
}
Expand Down Expand Up @@ -91,7 +91,7 @@ export const typeFilterIcons: Record<string, string> = {
Diffs: mdiPlusMinus,
}

export type FilterGroups = Record<Filter['kind'], SectionItem[]>
export type FilterGroups = Record<Filter['kind'], SectionItemData[]>

export function groupFilters(streamFilters: Filter[], selectedFilters: URLQueryFilter[]): FilterGroups {
const groupedFilters: FilterGroups = {
Expand Down
22 changes: 19 additions & 3 deletions client/web-sveltekit/src/routes/[...repo=reporev]/+layout.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,13 @@

import { page } from '$app/stores'
import { computeFit } from '$lib/dom'
import { getGraphQLClient } from '$lib/graphql'
import Icon from '$lib/Icon.svelte'
import GlobalHeaderPortal from '$lib/navigation/GlobalHeaderPortal.svelte'
import { DropdownMenu, MenuLink } from '$lib/wildcard'
import Popover from '$lib/Popover.svelte'
import RepoPopover, { fetchRepoPopoverData } from '$lib/repo/RepoPopover/RepoPopover.svelte'
import { delay } from '$lib/utils'
import { Alert, DropdownMenu, MenuLink } from '$lib/wildcard'

import type { LayoutData } from './$types'
import RepoSearchInput from './RepoSearchInput.svelte'
Expand Down Expand Up @@ -80,7 +84,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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, but I think we should save this for the planned header redesign

<svelte:fragment slot="content">
{#await delay(fetchRepoPopoverData(getGraphQLClient(), repoName), 200) then data}
<RepoPopover {data} withHeader />
{:catch error}
<Alert size="slim" variant="danger">{error}</Alert>
{/await}
</svelte:fragment>
</Popover>

<ul use:computeFit on:fit={event => (visibleNavEntries = event.detail.itemCount)}>
{#each navEntriesToShow as entry}
Expand Down Expand Up @@ -129,7 +142,7 @@
<style lang="scss">
nav {
display: flex;
align-items: baseline;
align-items: stretch;
gap: 0.5rem;
overflow: hidden;
flex: 1;
Expand All @@ -149,6 +162,9 @@
}

h1 {
display: flex;
align-items: center;

margin: 0 1rem 0 0;
font-size: 1rem;
white-space: nowrap;
Expand Down
Loading