[Discover] recently closed tabs preview#246973
Conversation
| } | ||
|
|
||
| export type RecentlyClosedTabItem = TabItem & { | ||
| closedAt: number; |
There was a problem hiding this comment.
This makes the dependency on closedAt more explicit. Previously, it was a shadow property that was being used but not disclosed in the unified tabs package interfaces.
|
|
||
| interface OptionData { | ||
| closedAt?: moment.Moment; | ||
| tabItem: RecentlyClosedTabItem; |
There was a problem hiding this comment.
Added so that it can be passed to getPreviewData
drewdaemon
added
release_note:enhancement
Team:DataDiscovery
| } | ||
| tabItem={{ id: option.key as string, label: option.label }} | ||
| previewData={previewData} | ||
| previewDelay={0} |
There was a problem hiding this comment.
Note this difference with the tab header previews. No delay felt better to me, but I'm not sure about it.
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
jughosta
left a comment
jughosta
left a comment
There was a problem hiding this comment.
Looks great, Drew! I like having the preview for closed tabs now 🎉
We could also enable it for "Opened tabs" list in the same popover, right?
| ) => { | ||
| const tabRuntimeState = selectTabRuntimeState(runtimeStateManager, tabState.id); | ||
|
|
||
| if (!tabRuntimeState) { |
There was a problem hiding this comment.
| if (!tabRuntimeState) { | |
| if (tabState.closedAt) { |
The normal tabs which are not visited yet also don't have a runtime state (after a page refresh for example).
| <> | ||
| {option.label} | ||
| {formattedTime && ( | ||
| <EuiText size="xs" color="subdued" className="eui-displayBlock"> | ||
| {formattedTime} | ||
| </EuiText> | ||
| )} | ||
| </> | ||
| ); |
There was a problem hiding this comment.
This part can be a reusable variable too, right?
| return tabItems.map((tab) => { | ||
| const closedAt = 'closedAt' in tab && tab.closedAt ? moment(tab.closedAt) : undefined; | ||
| const option = { | ||
| const momentClosedAt = moment(tab.closedAt); |
There was a problem hiding this comment.
Since the items get rerendered when the mouse cursor moves, the time updates too. Wdyt about this side effect?
There was a problem hiding this comment.
I think it's weird 😁 (will look into it)
There was a problem hiding this comment.
Fixed in 56237e6
Screen.Recording.2026-01-08.at.4.11.07.PM.mov
| title: `${tab.label}${ | ||
| momentClosedAt.isValid() ? ` (${momentClosedAt.format('LL LT')})` : '' | ||
| }`, |
There was a problem hiding this comment.
| title: `${tab.label}${ | |
| momentClosedAt.isValid() ? ` (${momentClosedAt.format('LL LT')})` : '' | |
| }`, |
The preview is sufficient I think, no?
Good idea 3a5dd80 |
jughosta
left a comment
jughosta
left a comment
There was a problem hiding this comment.
LGTM, thanks! 👍
Can we address the React warnings?
There was a problem hiding this comment.
There are a couple of warnings when opening the menu popover:
Warning: React does not recognize the `tabItem` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `tabitem` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
Warning: React does not recognize the `formattedTime` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `formattedtime` instead. If you accidentally passed it from a parent component, remove it from the DOM element.
There was a problem hiding this comment.
Hopefully. They may be created on the EUI side. Will take a look!
|
@drewdaemon Just a heads up that I pinged @l-suarez in Slack to do a design review on this after the weekend. |
|
sounds good davis, we will make it pretty before I merge 😁 |
|
Hello! Thanks for adding this to the tabs menu, super useful to see the query/dataview behind the tabs name. I created an instance in a la carte in case you need it: https://l-suarez-pr-246973-244863-recently-closed-tabs-preview.kbndev.co/ Ok, we had this issue with the tabs themselves when we were implementing them, the browsers default behaviour of opening a tooltip is not needed (we cover that use case with our own tabs preview), could we disable its appearance? Also, this is probably the components behaviour, but we don't display long names properly on the list. Truncation will be a nice to have here. In terms of alignment, could we align the preview like this so it doesn't overlap with the menu? That's all from me, looking great! |
|
Thinking about this work, it would be great if we can use the same pattern (showing query/dataview behind the tab on hover when the name is present) in other places, such as the dashboard panel list? I don't know how we're solving that particular work, but I thought this pattern would be nice to be reused somewhere else, and treat it as it is part of the Discover tab component pattern, wdyt @davismcphee @drewdaemon @jughosta? |
|
@l-suarez totally I'll look into the browser tooltip and the alignment. I also had the same thought about having this preview on the list when choosing a tab for the dashboard. Would have to think a bit about the technical side of that though because it's currently pretty coupled to the discover view. |
|
@elasticmachine merge upstream |
…daemon/kibana into 244863/recently-closed-tabs-preview
|
Thanks for the review @l-suarez! Regarding using the preview in the dashboard list too, it sounds reasonable to me, but I'm also unsure of the technical effort at a glance. We could include it in the designs though and investigate it as part of that project. @drewdaemon Thanks for the updates. I didn't do a formal review, but gave it a quick spin and the alignment looks much nicer! Just a heads up about a couple of things:
|
|
Davis — I haven't yet taken care of the design feedback. If you're hoping to review the design updates before this merges I can give you a ping when they're done! Barring an EUI change, we may be stuck with the native tooltip. It is EUI which is applying that I appreciate you pointing out the connection between the divs and the truncation. I added the first div you linked because with a span, the hover area for the preview popover is incomplete and buggy. So, I'll have to find a win-win. |
|
@elasticmachine merge upstream |
…daemon/kibana into 244863/recently-closed-tabs-preview
|
Ok, design feedback is addressed! |
⏳ Build in-progress, with failures
Failed CI StepsTest Failures
History
|
kibanamachine
added
backport:skip
6 participants
Summary
fix #244863
Checklist
release_note:*label is applied per the guidelinesRelease note:
Improves the recently closed tabs menu in Discover. On hover, you now see a preview of what was contained in each closed tab.