Skip to content

feat: small improvements to search #6736

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

Merged
AdityaHegde merged 4 commits into from
Feb 24, 2025
Merged

feat: small improvements to search #6736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff443ba
Limit the max values in leaderboard to 15
AdityaHegde Feb 21, 2025
1184258
Add loading and error state for search
AdityaHegde Feb 21, 2025
1b18781
Fix lint
AdityaHegde Feb 21, 2025
8def8bd
PR comments
AdityaHegde Feb 24, 2025
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
112 changes: 86 additions & 26 deletions web-common/src/features/dashboards/filters/dimension-filters/DimensionFilter.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import { Chip } from "@rilldata/web-common/components/chip";
import RemovableListBody from "@rilldata/web-common/components/chip/removable-list-chip/RemovableListBody.svelte";
import * as DropdownMenu from "@rilldata/web-common/components/dropdown-menu";
import SearchableMenuContent from "@rilldata/web-common/components/searchable-filter-menu/SearchableMenuContent.svelte";
import LoadingSpinner from "@rilldata/web-common/components/icons/LoadingSpinner.svelte";
import { Search } from "@rilldata/web-common/components/search";
import Tooltip from "@rilldata/web-common/components/tooltip/Tooltip.svelte";
import TooltipContent from "@rilldata/web-common/components/tooltip/TooltipContent.svelte";
import TooltipTitle from "@rilldata/web-common/components/tooltip/TooltipTitle.svelte";
Expand Down Expand Up @@ -40,13 +41,14 @@
timeEnd,
Boolean(timeControlsReady && open),
);
$: ({ data, error, isFetching } = $searchValues);

$: allSelected = Boolean(
selectedValues.length && $searchValues?.length === selectedValues.length,
selectedValues.length && data?.length === selectedValues.length,
);

function onToggleSelectAll() {
$searchValues?.forEach((dimensionValue) => {
data?.forEach((dimensionValue) => {
if (!allSelected && selectedValues.includes(dimensionValue)) return;

onSelect(dimensionValue);
Expand Down Expand Up @@ -113,29 +115,87 @@
</Tooltip>
</DropdownMenu.Trigger>

<SearchableMenuContent
{onSelect}
{onToggleSelectAll}
bind:searchText
showXForSelected={excludeMode}
selectedItems={[selectedValues]}
allowMultiSelect={true}
selectableGroups={[
{
name: "DIMENSIONS",
items: $searchValues.map((dimensionValue) => ({
name: dimensionValue,
label: dimensionValue,
})),
},
]}
<!-- There will be some custom controls for this. Until we have the full design have a custom dropdown here. -->
Copy link
Copy Markdown
Contributor

@ericpgreen2 ericpgreen2 Feb 21, 2025

Choose a reason for hiding this comment

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

I'm a little worried about divergence between our menu components. For example, all other SearchableMenuContent would also benefit from the loading & error states you've added here. I wonder if we could add the requisite functionality to SearchableMenuComponent (e.g. make it pluggable?). I'll leave it up to you.

Copy link
Copy Markdown
Collaborator Author

@AdityaHegde AdityaHegde Feb 24, 2025

Choose a reason for hiding this comment

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

Ya I wanted to wait for the design to stabilise before refactoring this too much. The end goal would be a common component with hooks for dimension filter.

<DropdownMenu.Content
align="start"
class="flex flex-col max-h-96 w-72 overflow-hidden p-0"
>
<Button slot="action" on:click={onToggleFilterMode} type="secondary">
{#if excludeMode}
Include
{:else}
Exclude
<div class="px-3 pt-3 pb-1">
<Search
bind:value={searchText}
label="Search list"
showBorderOnFocus={false}
/>
</div>

<div class="flex flex-col flex-1 overflow-y-auto w-full h-fit pb-1">
{#if isFetching}
<div class="min-h-9 flex flex-row items-center mx-auto">
<LoadingSpinner />
</div>
{:else if error}
<div class="min-h-9 p-3 text-center text-red-600 text-xs">
{error}
</div>
{:else if data}
<DropdownMenu.Group class="px-1">
{#each data as name (name)}
{@const selected = selectedValues.includes(name)}

<DropdownMenu.CheckboxItem
class="text-xs cursor-pointer"
role="menuitem"
checked={selected}
showXForSelected={excludeMode}
on:click={() => onSelect(name)}
>
<span>
{#if name.length > 240}
{name.slice(0, 240)}...
{:else}
{name}
{/if}
</span>
</DropdownMenu.CheckboxItem>
{:else}
<div class="ui-copy-disabled text-center p-2 w-full">
no results
</div>
{/each}
</DropdownMenu.Group>
{/if}
</Button>
</SearchableMenuContent>
</div>

<footer>
<Button on:click={onToggleSelectAll} type="plain">
{#if allSelected}
Deselect all
{:else}
Select all
{/if}
</Button>
<Button on:click={onToggleFilterMode} type="secondary">
{#if excludeMode}
Include
{:else}
Exclude
{/if}
</Button>
</footer>
</DropdownMenu.Content>
</DropdownMenu.Root>

<style lang="postcss">
footer {
height: 42px;
@apply border-t border-slate-300;
@apply bg-slate-100;
@apply flex flex-row flex-none items-center justify-end;
@apply gap-x-2 p-2 px-3.5;
}

footer:is(.dark) {
@apply bg-gray-800;
@apply border-gray-700;
}
</style>
35 changes: 26 additions & 9 deletions web-common/src/features/dashboards/filters/dimension-filters/dimensionFilterValues.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { CompoundQueryResult } from "@rilldata/web-common/features/compound-query-result";
import {
createInExpression,
createLikeExpression,
Expand All @@ -13,7 +14,7 @@ export function useDimensionSearch(
timeStart?: string,
timeEnd?: string,
enabled?: boolean,
) {
): CompoundQueryResult<string[]> {
const addNull = searchText.length !== 0 && "null".includes(searchText);

const queries = metricsViewNames.map((mvName) =>
Expand All @@ -37,15 +38,31 @@ export function useDimensionSearch(
);

return derived(queries, ($queries) => {
const someQueryFetching = $queries.some((q) => q.isFetching);
Copy link
Copy Markdown
Contributor

@ericpgreen2 ericpgreen2 Feb 21, 2025

Choose a reason for hiding this comment

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

Typically we should use isLoading not isFetching

Copy link
Copy Markdown
Collaborator Author

@AdityaHegde AdityaHegde Feb 24, 2025

Choose a reason for hiding this comment

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

Hmmm my understanding was isLoading is true only on the 1st fetch. But i guess it is for the 1st fetch of the query object. Since we create a new one on every searchText it will be true for showing the spinner.

Copy link
Copy Markdown
Collaborator Author

@AdityaHegde AdityaHegde Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But if for whatever reason there a refetch the data would go from blank to showing values without a spinner. So I think it is fine to use isFetching here. With tanstack v5 migration we wont need explicit compound query then we can revisit this.

if (someQueryFetching) {
return {
data: undefined,
error: undefined,
isFetching: true,
};
}
const errors = $queries.filter((q) => q.isError).map((q) => q.error);
if (errors.length > 0) {
return {
data: undefined,
// TODO: merge multiple errors
error: errors[0]?.response?.data.message,
isFetching: false,
};
}

const items = $queries.flatMap((query) => query.data?.data || []);
const values = items.map((item) => item[dimensionName] as string);
const seen = new Set();
return values.filter((value) => {
if (seen.has(value)) {
return false;
}
seen.add(value);
return true;
});
const dedupedValues = new Set(values);
return {
data: [...dedupedValues],
error: undefined,
isFetching: false,
};
});
}
9 changes: 8 additions & 1 deletion web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
const slice = 7;
const gutterWidth = 24;
const queryLimit = 8;
const maxValuesToShow = 15;

export let dimension: MetricsViewSpecDimensionV2;
export let timeRange: V1TimeRange;
Expand Down Expand Up @@ -197,6 +198,7 @@
leaderboardTotal,
));

$: belowTheFoldDataLimit = maxValuesToShow - aboveTheFold.length;
$: belowTheFoldDataQuery = createQueryServiceMetricsViewAggregation(
instanceId,
metricsViewName,
Expand All @@ -221,10 +223,15 @@
timeRange,
comparisonTimeRange,
measures,
limit: belowTheFoldDataLimit.toString(),
},
{
query: {
enabled: !!belowTheFoldValues.length && timeControlsReady && visible,
enabled:
!!belowTheFoldValues.length &&
timeControlsReady &&
visible &&
belowTheFoldDataLimit > 0,
},
},
);
Expand Down
Loading