-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(data-exploration): fix event table regressions #13251
Conversation
…ng to an old thing)
This reverts commit 360b0a4.
This is ready for a look. The cypress tests fail for some reason, I suspect weird caching coming from the node18 or pnpm changes. |
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.
This is almost perfect now, but would be great to keep those date separators in the new component. :D
Also, it seems the event buffer notice isn't there, though I'm totally OK with doing away with that as long as we only release data-exploration-live-events
after Pipeline folks' "live events" table is live.
frontend/src/lib/utils.tsx
Outdated
@@ -1542,3 +1542,5 @@ export function downloadFile(file: File): void { | |||
link?.parentNode?.removeChild(link) | |||
}, 0) | |||
} | |||
|
|||
export const daysAgo = (days: number): string => now().subtract(days, 'day').toISOString() |
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.
Does this truncate to start of day?
As it is now, TBH I'd find the plain now().subtract(days, 'day').toISOString()
clearer, because then it's pretty evident what's going on
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.
I stole it from the existing events table, and ran into some mocking trouble, so didn't reuse the new lib there. Good idea though, I just inlined this now.
// NOTE: PropertyFilterType.Event is not a mistake. PropertyKeyInfo only knows events vs elements ¯\_(ツ)_/¯ | ||
return { title: <PropertyKeyInfo value={key.substring(18)} type={PropertyFilterType.Event} disableIcon /> } | ||
} else { | ||
return { title: String(key) } |
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.
Supposedly key
already must be string
, so isn't useful to stringify it (unless it can be something else, but then typing should reflect that for everyone's safety)
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.
I can only assume there's a historic reason for this...
Trying to clear cache.
…to event-data-table-fixr
Problem
Fixes #13223
Changes
width:0
How did you test this code?
Clicky click.