-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[Discover] [Unified Tabs] Add relative time info when tab was closed #244887
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
base: main
Are you sure you want to change the base?
[Discover] [Unified Tabs] Add relative time info when tab was closed #244887
Conversation
|
/ci |
|
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
| const menuButton = await screen.findByTestId(tabsBarMenuButtonTestId); | ||
| await user.click(menuButton); | ||
|
|
||
| expect(await screen.findByText('Recently closed')).toBeInTheDocument(); | ||
| expect(await screen.findByText(/5 minutes ago/i)).toBeInTheDocument(); | ||
| expect(await screen.findByText(/10 minutes ago/i)).toBeInTheDocument(); |
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.
do you need them to be findByTestId or can they be getByTestId?
| const menuButton = await screen.findByTestId(tabsBarMenuButtonTestId); | |
| await user.click(menuButton); | |
| expect(await screen.findByText('Recently closed')).toBeInTheDocument(); | |
| expect(await screen.findByText(/5 minutes ago/i)).toBeInTheDocument(); | |
| expect(await screen.findByText(/10 minutes ago/i)).toBeInTheDocument(); | |
| const menuButton = screen.getByTestId(tabsBarMenuButtonTestId); | |
| await user.click(menuButton); | |
| expect(screen.getByTestId('Recently closed')).toBeInTheDocument(); | |
| expect(screen.getByTestId(/5 minutes ago/i)).toBeInTheDocument(); | |
| expect(screen.getByTestId(/10 minutes ago/i)).toBeInTheDocument(); |
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.
Thanks for the suggestion Alex! Tbh I was just following the same pattern as the whole file is using. It's perfectly fine this way though, would you like me to update the whole file?
On a separate note - was replacing findByText by getByTestId too unintentional in your suggestion or it was on purpose?
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.
No worries about the whole file, I guess this ones are fine for now 😄
Sorry no! I I just did a quick review and thought everything was byTestId - I guess it's just getByXX instead of findByXX 🙈
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.
Thought so, thanks for clarifying 😁
I'll adjust the getter, thanks!
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.
Addressed in 43acb5c
💚 Build Succeeded
Metrics [docs]Async chunks
History
|
Summary
Resolves: #244365
This pull request improves the handling and display of recently closed tabs in the
TabsBarMenucomponent. The main changes include enhancing how closed timestamps are managed and displayed, and updating the UI to show relative time for closed tabs.Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.