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

refac: integrate canvas resolver API #6411

Merged
merged 7 commits into from
Jan 16, 2025
Merged

Conversation

djbarnwal
Copy link
Member

@djbarnwal djbarnwal commented Jan 14, 2025

Integrate canvas resolver API and newly added properties to the YAML.

  • Integrate the new resolved canvas API
  • Create new class for spec readables
  • Add keys and style canvas page panel as per mocks
  • Add all time range store

There are more changes and refactoring needed for time filters. Those will be addressed in a different PR.

@djbarnwal djbarnwal marked this pull request as ready for review January 15, 2025 11:27
web-common/src/features/canvas/selector.ts Outdated Show resolved Hide resolved
web-common/src/features/canvas/stores/canvas-entities.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.

It seems we have two layers of selector functions:

  1. In the select function provided to the ResolveCanvas API's TanStack Query request
  2. The functions provided to the derived stores in this file

Maybe consider if this should be a one-tier system, not two-tier? I wonder if the TanStack Query call and all selectors could all be co-located in this class.

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 wonder if there's any performance cost of creating a derivable out of query response. The reason I chose to use derivables instead of select function was the ease of integration in the components. All these selectors derive from a single query so we wouldn't have to check for fetch status or error at every place the selectors are used.

I have added more methods to canvas-spec.ts in #6396 I have fixed the nits in this PR. We can continue this discussion around selection there.

@djbarnwal djbarnwal merged commit b6baa76 into main Jan 16, 2025
7 checks passed
@djbarnwal djbarnwal deleted the refac/intergrate-canvas-api branch January 16, 2025 10:23
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