[ES|QL Controls] Highlight related panels on click, refactor Related Panels system#264426
[ES|QL Controls] Highlight related panels on click, refactor Related Panels system#264426Zacqary wants to merge 55 commits into
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Zacqary
force-pushed
the
204508-esql-highlight
branch
from
b7cc28a to
087ca94
Compare
Zacqary
added
release_note:enhancement
Team:Presentation
Zacqary
force-pushed
the
204508-esql-highlight
branch
from
087ca94 to
4021c2d
Compare
Zacqary
force-pushed
the
204508-esql-highlight
branch
from
4021c2d to
d3b43ad
Compare
Zacqary
force-pushed
the
204508-esql-highlight
branch
2 times, most recently
from
8b4343b to
ea0fda0
Compare
Zacqary
force-pushed
the
204508-esql-highlight
branch
9 times, most recently
from
5270bb4 to
5aee652
Compare
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
andrimal
left a comment
andrimal
left a comment
There was a problem hiding this comment.
Vis changes LGTM! (only one mock file updated)
|
@florent-leborgne could we get your input of the copy for the case where a ES|QL variable is not being used by any (ES|QL) visualization? 
|
florent-leborgne
left a comment
florent-leborgne
left a comment
There was a problem hiding this comment.
Quick copy check based on @andreadelrio's request
…ents/control_label_tooltip.tsx Co-authored-by: Florent LB <florent.leborgne@elastic.co>
andreadelrio
left a comment
andreadelrio
left a comment
There was a problem hiding this comment.
Design changes LGTM
…ents/control_label_tooltip.tsx Co-authored-by: Florent LB <florent.leborgne@elastic.co>
There was a problem hiding this comment.
Do we want to blur unrelated panels when selecting a control? If so - maybe just the blur on unrelated panels is enough without the border around related panels? Then the teal border can apply just to the selected control. This feels more consistent with the editing experience IMO cc @andreadelrio
Screen.Recording.2026-05-22.at.1.55.38.PM.mov
There was a problem hiding this comment.
@Heenawter I see what you mean and yes, I think this would be a valid way to simplify the styles. The blur of unrelated panels is probably enough.
|
|
||
| export const useIndicateRelatedPanelsSelector = ( | ||
| api: unknown, | ||
| skipDebounce?: boolean // For faster testing |
There was a problem hiding this comment.
Is this relevant anymore?
| setIndicateRelatedPanelsId: (panelId?: string) => void; | ||
| indicateRelatedPanelsId$: BehaviorSubject<string | undefined>; |
There was a problem hiding this comment.
| setIndicateRelatedPanelsId: (panelId?: string) => void; | |
| indicateRelatedPanelsId$: BehaviorSubject<string | undefined>; | |
| setRelatedPanelIds: (panelId?: string) => void; | |
| relatedPanelIds$: BehaviorSubject<string | undefined>; |
There was a problem hiding this comment.
These refer to the ID of the panel that's been selected to do the indicating, not a list of panel IDs that are related. I'll see if I can come up with a clearer name.
| export interface PresentationContainer<ApiType extends unknown = unknown> extends CanAddNewPanel { | ||
| export interface PresentationContainer<ApiType extends unknown = unknown> | ||
| extends CanAddNewPanel, | ||
| Partial<CanIndicateRelatedChildren> { |
There was a problem hiding this comment.
I am not convinced that this should be part of PresentationContainer rather than just the Dashboard API.... Is there a reason you put it here?
There was a problem hiding this comment.
I was considering that Discover wants to use related panels at some point. We've decided that's not a strong design goal anymore, but I think this is a low-impact way to make that easier.
There was a problem hiding this comment.
I personally still think it would be cleaner and easier to understand if the Dashboard API implemented this rather than PresentationContainer. After all, PresentationContainer doesn't have all of the highlightedPanels$ stuff - this is just on Dashboard - and they seem closely related to me, especially now that a bulk of the logic for related panels is in track_panel :)
| const tooltipLabel$ = new BehaviorSubject<string>(state.title ?? ''); | ||
| const tooltipLabelSubscription = combineLatest([ | ||
| selections.api.esqlVariable$, | ||
| labelManager.api.label$, | ||
| ]) | ||
| .pipe( | ||
| map(([{ key: variableName, type: variableType }, label]) => { | ||
| return getTooltipTitle(variableName, variableType, label); | ||
| }) | ||
| ) | ||
| .subscribe((next) => tooltipLabel$.next(next)); |
There was a problem hiding this comment.
This feels like overkill to me. Can we not just calculate this in the ControlLabelTooltip component? It doesn't need to change on-the-fly, so it can be recomputed each time the tooltip renders; maybe we could even just do something simple like a getter in the API rather than a subject that we need to subscribe to?
There was a problem hiding this comment.
I'll test it again to make sure I didn't miss something but I believe EUI tooltips don't actually re-render the way we expect them to, which is why we need to explicitly pipe a new tooltip into them on change
There was a problem hiding this comment.
Sorry, it's been a while since I did this and forgot the context. This isn't to deal with a rendering issue, it's to keep ControlLabelTooltip generic and letting the ES|QL control handle formatting ES|QL variables names.
Note what getTooltipTitle does:
export const getTooltipTitle = (
variableName: string,
variableType: ESQLVariableType,
label?: string
) => {
const variableWithPrefix = `${getVariableNamePrefix(variableType)}${variableName}`;
return label && label !== variableName ? `${label} (${variableWithPrefix})` : variableWithPrefix;
};It's formatting the tooltip title as Control Name (?variable_name). This is an ES|QL control concern only, and we want to keep ControlLabelTooltip free to theoretically handle tooltips for other control types in the future.
BehaviorSubjects are the idiomatic way that controls output their titles and labels even though they only change on edit, so that's what I went with here.
| if (!parentApiSubscription.current) { | ||
| const sub = combineLatest([ | ||
| parentApiLoaded.viewMode$, | ||
| parentApiLoaded.indicateRelatedPanelsId$, | ||
| api.relatedPanels$, | ||
| ]).subscribe(([vm, indicateId, relatedPanelIds]) => { | ||
| setViewMode(vm); | ||
| setIndicateRelatedPanelsId(indicateId); | ||
| setRelatedPanels(relatedPanelIds); | ||
| }); | ||
| parentApiSubscription.current = sub; | ||
| } |
There was a problem hiding this comment.
Why not just use useBatchedPublishingSubjects?
There was a problem hiding this comment.
Let me give that a try. It didn't work before the refactor that eliminated the related panels manager but with the reduced complexity we might be able to do that
There was a problem hiding this comment.
Ok yeah it's the parentApiLoaded issue. ControlPanel is guaranteed to have parentApi available. ConditionalLabelWrapper is not, and needs to derive parentApi from its passed api. But in my testing, parentApi doesn't immediately get attached to api when an unpinned panel renders.
I think this is counter-intuitively the simplest way to handle it.
💔 Build Failed
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
|
10 participants
Summary
Closes #204508
Refactor
Removes the Related Panels Manager from the dashboard API and replaces this with a
relatedPanels$publishing subject on all panels. All panels now compute which of their siblings they're related to.Enhancement
Allows the user to click on ES|QL control labels to highlight all their related panels. Also fixes a bug where giving an ES|QL control a
titledid not save when saving the dashboard.And displays a warning on ES|QL controls with no related panels:
Chained ES|QL controls (a control that references the variable of another control) are also supported:
Implementation notes
CanIndicateRelatedChildren, only ES|QL controls currently publishCanIndicateRelatedSiblingsso as not to enable this behavior on filter controlsTesting
Click to expand
Checklist