refactor Leaderboard/DimensionTable queries #5942
Merged
+622
−632
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
briangregoryholmes
changed the title
AdityaHegde
requested changes
Oct 23, 2024
web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
Outdated
Show resolved
Hide resolved
web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
Outdated
Show resolved
Hide resolved
|
Is this one ready for review again? IMO this is an important one to get in the next release |
AdityaHegde
reviewed
Oct 29, 2024
web-common/src/features/dashboards/leaderboard/Leaderboard.svelte
Outdated
Show resolved
Hide resolved
AdityaHegde
reviewed
Oct 30, 2024
There was a problem hiding this comment.
Seeing some weird behaviour still,
- Selecting below fold values, refreshing, expanding the dimension and coming back removes the values.
https://www.loom.com/share/41ba9ec4e6b84956859aabcc4d4b5ca1?sid=267e6432-8c69-49eb-bc76-2b94aa3a343d - I feel like there is some reactivity missing somewhere. Values do not update when I select filters for other dimensions. Also when values go above the fold they are duplicated.
https://www.loom.com/share/d1d8311b050145089c3a9592598b339d?sid=089fc062-7494-4f5d-b421-c1bd15c983e8
briangregoryholmes
changed the title
|
I've decided to just make this PR the full refactor. These changes have resolved your two notes, but I recommend taking another look now that the scope has changed. |
AdityaHegde
approved these changes
Nov 6, 2024
| @@ -1,181 +1,17 @@ | |||
| import { mergeMeasureFilters } from "@rilldata/web-common/features/dashboards/filters/measure-filters/measure-filter-utils"; | |||
There was a problem hiding this comment.
Nice seeing this massive removal! This was needed when we had different APIs used. Now with a single AggregationRequest I was thinking of writing up a wrapper around it using builder pattern. Might decrease the code a lot more.
|
Added a catch for that situation. Those values are now displayed below the fold with no data. |
AdityaHegde
approved these changes
Nov 7, 2024
| let selectedBelowTheFold: LeaderboardItemData[] = []; | ||
| let showExpandTable = false; | ||
| let noAvailableValues = true; | ||
| $: console.log(sortedData?.data); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the way queries are handled within the
LeaderboardandDimensionTablecomponents. Namely, the reliance on externally derived readables that are accessible only within the StateManger context has been removed. Instead, they accept state-agnostic props and it is the responsibility of the parent component to pass them down accordingly. The logic for building out the queries/options objects is now collocated with the relevant component. In the future,LeaderboardandDimensionTablewill be consolidated into a single component (along withLeaderboardDisplayandDimensionTableDisplay).As part of this work, the
Leaderboardnow properly displays data for items "below the fold", as seen below.Closes #5753