AdHocVariable: Edit injected filters #1062
Conversation
mdvictor
added
minor
| filtersVar._updateFilter(filtersVar.state.filters[0], { key: 'newKey', keyLabel: 'newKey' }); | ||
| }); | ||
|
|
||
| // injected filters stored in the following format: normal|adhoc|values\original|values\filterOrigin |
There was a problem hiding this comment.
I think the format is fine, question is if you sync them always? I think we should probably sync them only when they have changed, otherwise one will navigate to a dashboard in the same scope.
There was a problem hiding this comment.
I think you are right. I kept thinking about this and initially thought that both normal filters and baseFilters with an origin should be treated the same, thus having these baseFilters in the URL, same as your average filter, but I agree that there is no point in having both unmodified scope injected filter and the scope itself in there
| private getInjectedKey(): string { | ||
| return `var-injected-${this._variable.state.name}`; | ||
| } |
There was a problem hiding this comment.
Why do we need to do this? I think we could sync the injected alongside the regular filters (keys that are injected can't be really overwritten by the user) and figure out which filters were scope-injected based on keys matching and origin. WDYT?
There was a problem hiding this comment.
Indeed, no need for another key other than for verbosity, we can identify injected filters based on origin. Removing the extra key
There was a problem hiding this comment.
Looking great!
Some safeguards are needed for editing scope injected filters:
- prevent editing
operatorusingshift+tabkeyboard shortcut - prevent editing
operatorusing backspace (when deleting last value) - deleting
operatortriggers focus shift to wip (add new) filter and scopes filter stays in broken edit mode - clicking on scope inject filter to edit and then clicking on
operatorpill leaves scope injected filter in edit mode and focuses on wip filter - backspace from wip filter does not start editing scope injected filters
Screen.Recording.2025-03-13.at.16.07.45.mov
Screen.Recording.2025-03-13.at.16.07.04.mov
Other than those, looks good ✅
| if (baseFilters?.length) { | ||
| value.push( | ||
| ...baseFilters | ||
| ?.filter(isFilterComplete) | ||
| .filter((filter) => !filter.hidden && filter.origin && filter.originalValue) | ||
| .map((filter) => | ||
| toArray(filter) | ||
| .map(escapeInjectedFilterUrlDelimiters) | ||
| .join('|') | ||
| .concat( | ||
| `#${filter.originalValue?.map(escapeInjectedFilterUrlDelimiters).join('|') ?? ''}#${filter.origin}` | ||
| ) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This part could use some comments
There was a problem hiding this comment.
I added a comment explaining the url sync format for injected filters
| {filter.origin && !filter.originalValue && ( | ||
| <IconButton | ||
| name="info-circle" | ||
| size="md" | ||
| className={styles.pillIcon} | ||
| tooltip={`This is a ${filter.origin} injected filter`} | ||
| /> | ||
| )} | ||
|
|
||
| {filter.origin && filter.originalValue && ( | ||
| <IconButton | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| model.restoreOriginalFilter(filter); | ||
| }} | ||
| name="history" | ||
| size="md" | ||
| className={styles.pillIcon} | ||
| tooltip={`Restore filter to its original value`} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
nit: could be combined into:
{filter.origin && filter.originalValue ? (
<IconButton
onClick={(e) => {
e.stopPropagation();
model.restoreOriginalFilter(filter);
}}
name="history"
size="md"
className={styles.pillIcon}
tooltip={`Restore filter to its original value`}
/>
) : (
<IconButton
name="info-circle"
size="md"
className={styles.pillIcon}
tooltip={`This is a ${filter.origin} injected filter`}
/>
)}
| @@ -37,6 +38,7 @@ export interface AdHocFilterWithLabels<M extends Record<string, any> = {}> exten | |||
| // filter origin, it can be either scopes, dashboards or undefined, | |||
| // which means it won't appear in the UI | |||
| origin?: FilterOrigin; | |||
| originalValue?: string[]; | |||
There was a problem hiding this comment.
I find it hard to understand what this is and why it should display the history icon when it is defined. My initial thought was that it is defined when not edited, not the other way around.
There was a problem hiding this comment.
I see what you mean, this could use better naming, maybe something like savedOriginalValue? The point of the property is indeed to hold the initial filter value before it is being edited
| ...(state.baseFilters?.filter((filter) => filter.origin) ?? []), | ||
| ...(state.filters ?? []), |
There was a problem hiding this comment.
This is being used a three different places? Would it make sense to extract this into a helper that describes what it does?
| // if the scope filters contain the key of an edited scope one, we replace | ||
| // with the edited one, otherwise if an edited filter is not found in | ||
| // the scope filters (this can happend when changing scopes alltogether) | ||
| // we drop the edited one and use the new filters from the new scopes |
There was a problem hiding this comment.
Isn't this similar to what's being done in _updateFilter? I am not familiar with the order of things, how come it is needed to be done in both places?
There was a problem hiding this comment.
Not sure I follow what you mean by being similar. This one sets the entire filters list based on scope changes.
|
🚀 PR was released in |
Allows adhocs to show and edit base filters if they have an origin.
Holds base filters into url sync to allow for passing them over from one dashboard to another, alongside the source and original value of a injected filter.
📦 Published PR as canary version:
6.5.0--canary.1062.13950672491.0✨ Test out this PR locally via: