-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Allows mustache syntax in numeric filters #14220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I misreading, or this function calling emit('input', val)
for every case? If that's true, could this function not just simply be:
function emitValue(val: string) {
if (val === '') return emit('input', null);
emit('input', val);
}
is there a time when it wouldn't match: if (typeof val !== 'string' || new RegExp(inputPattern.value).test(val)) {
return emit('input', val);
} UPDATE: Looks like it was changed in this PR #9376 To allow for dynamic variables |
…es/14198-system-filter
@@ -152,7 +152,7 @@ export default defineComponent({ | |||
return { displayValue, width, t, emitValue, inputEl, inputPattern, dateTimeMenu }; | |||
|
|||
function emitValue(val: unknown) { | |||
const mustache = new RegExp(/({{.*?}})/g); | |||
const mustache = new RegExp(/^({{.*?}})$/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rijkvanzanten Should "multi tags" be allowable in the system filter context for insights? Eg: users can form custom strings {{var1}}_{{var2}}
.
I wonder if a property should be added to allowVariables
on system-filter.vue
as it is currently only supported in insights. 🤔
The regex could be updated to along the lines of ({{[\w\d_]+?}})
as only alphanumeric characters and underscores are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex should definitely be updated at the very least
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seeing we're now using this interface inside insights options as well, it should support the multi-var style too yeah
…es/14198-system-filter
Heya, thanks for your PR! Our small team has been attempting to work through a backlog of external contributions for the better part of a year, but in that time many have become too stale to merge or the change is no longer desired/relevant.. |
Description
Before:
Axis.-.Google.Chrome.2022-06-29.21-37-51.mp4
After:
Screen.Recording.2022-06-30.at.11.02.00.AM.mov
Note: This will allow mustache syntax across the entire app's use of system-filter even though this syntax is really only valuable to the Insights dashboard. If this is not ideal I can look for a different solution.
Fixes #14198
Type of Change
Requirements Checklist
If adding a new feature: