feat(LabelsView): Include/exclude panel actions #210
Conversation
Unit test coverage
|
src/pages/ProfilesExplorerView/domain/variables/FiltersVariable/filters-ops.tsx
Outdated
Show resolved
Hide resolved
# Conflicts: # src/pages/ProfilesExplorerView/domain/variables/FiltersVariable/filters-ops.tsx
| @@ -176,16 +176,6 @@ test.describe('Labels view', () => { | |||
| stylePath: './e2e/fixtures/css/hide-all-controls.css', | |||
| }); | |||
| }); | |||
|
|
|||
| test('Add to filters action', async ({ exploreProfilesPage }) => { | |||
There was a problem hiding this comment.
Exchanged for unit tests below
| import { isRegexOperator } from '@shared/components/QueryBuilder/domain/helpers/isRegexOperator'; | ||
| import { CompleteFilter, OperatorKind } from '@shared/components/QueryBuilder/domain/types'; | ||
|
|
||
| export const convertPyroscopeToVariableFilter = (filter: CompleteFilter): AdHocVariableFilter => { |
There was a problem hiding this comment.
Because the .tsx extension has been changed, this file appears as new. But what this PR did is to keep this function and to add the 3 new functions below.
|
Works really nice! I'll have a look a the code in a bit. There's one case that I found confusing:
Screen.Recording.2024-10-31.at.11.11.45.movIt was also confusing that after including I need to click twice to actually exclude the same filter: Screen.Recording.2024-10-31.at.11.13.02.mov |
...upByLabels/components/SceneLabelValuesGrid/infrastructure/buildLabelValuesGridQueryRunner.ts
Show resolved
Hide resolved
src/pages/ProfilesExplorerView/domain/variables/FiltersVariable/__tests__/filter-ops.spec.ts
Show resolved
Hide resolved
| filters | ||
| .filter((f) => f.key === key && (f.operator === '=~' || f.operator === '!~')) | ||
| .some((f) => { | ||
| if (!f.value.split('|').includes(value)) { |
There was a problem hiding this comment.
note: Probably gonna work in 99% of cases but it may lead to weird edge-case scenarios, e.g.
Screen.Recording.2024-10-31.at.11.25.45.mov
There was a problem hiding this comment.
Good catch, I guess the query builder should strip the ()in this case
...s/components/SceneGroupByLabels/domain/actions/IncludeExcludeAction/IncludeExcludeAction.tsx
Show resolved
Hide resolved
|
I would also be great to add some user tracking 👍 |
I think the 1st attempt was too complex. So I simplified the logic to make the UI more intuitive IMO... In summary:
|
Done! |
✨ Description
Related issue(s): solves #204
Similarly to what the Explore Logs app provides to easily manipulate filters (e.g. see the "Patterns" tab in the Grafana's playground), this PR introduces the "Include" and "Exclude" buttons on the "Labels" view:
📖 Summary of the changes
This PR introduces the new
IncludeExcludeActionScene action, responsible for publishing an event when one of its button is clicked. In turn, this event is captured by their ancestorSceneGroupByLabels, which is responsible for updating the app filters.Note that the app filters remain the only source of truth: the status of the "Include" / "Exclude" buttons is determined by parsing the app filters. To mitigate performance issues when there are many buttons rendered on the screen, this PR uses memoization.
See diff tab for specific comments.
🧪 How to test?