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.

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

feat: dimension and measure filter for canvas #6396

Merged
merged 36 commits into from
Jan 21, 2025
Merged

Conversation

djbarnwal
Copy link
Member

@djbarnwal djbarnwal commented Jan 12, 2025

Adds dimension and measure filters for canvas.

Merge after #6411

TODOs

  • Cleanup
  • Support selection of multiple metrics view in filter bar

@djbarnwal djbarnwal self-assigned this Jan 13, 2025
Copy link
Contributor

@ericpgreen2 ericpgreen2 left a comment

Choose a reason for hiding this comment

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

Left some comments, but generally LGTM!

web-common/src/features/canvas/components/selectors.ts Outdated Show resolved Hide resolved
web-common/src/features/dashboards/filters/Filters.svelte Outdated Show resolved Hide resolved
web-common/src/features/dashboards/stores/filter-utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

@briangregoryholmes is your proposal that we lift the StateManagers even further up the component tree, or keep them here?

@djbarnwal djbarnwal requested a review from ericpgreen2 January 20, 2025 11:38
@djbarnwal djbarnwal merged commit 923486d into main Jan 21, 2025
7 checks passed
@djbarnwal djbarnwal deleted the feat/canvas-filters branch January 21, 2025 18:05
Comment on lines +246 to +252
void queryClient.refetchQueries(
getQueryServiceResolveCanvasQueryKey(
this.instanceId,
res.name.name,
{},
),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we use invalidateQueries, though it might not work due to this: https://github.com/rilldata/rill-private-issues/issues/719

} from "@rilldata/web-common/runtime-client";
import { derived, type Readable } from "svelte/store";
import { derived, get, type Readable } from "svelte/store";

export class CanvasResolvedSpec {
canvasSpec: Readable<V1CanvasSpec | undefined>;
isLoading: Readable<boolean>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need isError and error for error handling?

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, might it be easier to expose the $validSpecStore itself, rather than duplicating the fields that map 1:1 to the TanStack Query response? If so, maybe the exposure of the useCanvas response belongs in the top-level CanvasEntity class?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants