AdhocFilters: Show label for selected key #690
Conversation
ashharrison90
added
patch
| return toSelectableValue(matchingDefaultKey); | ||
| } | ||
| if (state.keys) { | ||
| return state.keys.find(option => option.value === filter.key); |
There was a problem hiding this comment.
are we concerned about having to find the option in the (potentially large) list here? 🤔
|
@ashharrison90 - think this ain't sufficient, as it will still use I think we need to trigger value "validation" on acivation when there are filters that were synced from URL. For that we need to fetch the keys from the data source and upate the available options accordingly. @torkelo - any reason why we don't do that already? |
|
AdHocFilters are not really well designed for "display labels" for keys, as we only include the value in URL and loading the filter state from URL should be all that is needed before queries are issued and state sync Do not want the AdHocFilterRenderer to have to issue a query for every filter to lookup the label, or would be nice to avoid that, just so messy |
bfmatei
force-pushed
the
ash/adhoc-filter-label
branch
from
e7a6b10
to
19a7c56
Compare
|
@torkelo @dprokop @ashharrison90 I just pushed an iteration of syncing the label via the URL. It works like this:
One thing to have in mind: extending Also a small demo: Screen.Recording.2024-04-17.at.15.47.05.mov |
| ); | ||
|
|
||
| act(() => { | ||
| locationService.push('/?var-filters=newKey|=|newValue,New Value&var-filters=newKey2|=~|newValue2,New Value 2'); |
There was a problem hiding this comment.
Can we add a test where key and value have a "," in them? btw, can't we use | (pipe) character as separator so we can use one split operation?
There was a problem hiding this comment.
ah, I see now why we need two delimiters (as both key and value can have an optional label)
There was a problem hiding this comment.
The optional label was more of an afterthought. The initial idea was to offer backwards compatibility with the existing URLs.
I also pushed changes to the comma test where new the value also contains a comma.
There was a problem hiding this comment.
this would not be backward compatible with old URLs (in the sense new URL would break on old Grafana/scenes)
There was a problem hiding this comment.
Ah, the idea was the other way around: offer compatibility for the existing URLs.
In the new implementation you can still load an URL that was bookmarked in an older Grafana/scenes version. There's only one exception to this though: values saved in the past and containing commas would break the functionality in the new implementation as they were not encoded to __gfc__. Changing the key name would overcome this but I think it's way too much hassle for such edge case.
| }); | ||
|
|
||
| expect(locationService.getLocation().search).toBe( | ||
| '?var-filters=newKey%7C%3D%7CnewValue,New%20Value&var-filters=key2%7C%3D%7Cval2' |
There was a problem hiding this comment.
Think we should encode , shouldn't we?
There was a problem hiding this comment.
Good question. I would have encoded the comma in the tests but the problem is that Scenes does not encode it for some reason. In the demo you can see that when adding it to the URL it is not encoded.
bfmatei
force-pushed
the
ash/adhoc-filter-label
branch
from
7002fd9
to
3e7c6ac
Compare
|
🚀 PR was released in |
Ensures we find the selected option from the list and get the full key/value object instead of creating a new object from just the key
📦 Published PR as canary version:
4.11.3--canary.690.8783271240.0✨ Test out this PR locally via:
npm install @grafana/scenes@4.11.3--canary.690.8783271240.0 # or yarn add @grafana/scenes@4.11.3--canary.690.8783271240.0