-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[Connector Builder] Add ability to click on logs to expand them #19195
Changes from 5 commits
605abed
518b13c
10330ca
7cba615
011bed4
774e70b
ea464f2
4749c20
800c675
98b03fa
85c479d
7ace6f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,3 +35,16 @@ | |
max-height: 100%; | ||
overflow-y: auto; | ||
} | ||
|
||
// add a fade at the bottom of the tabPanel | ||
.tabPanel::after { | ||
content: ""; | ||
position: absolute; | ||
z-index: 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually tend to maintain all z-indexes (since they are a global numbering system across all of the webapp) in the |
||
bottom: 0; | ||
left: 0; | ||
pointer-events: none; | ||
background-image: linear-gradient(to bottom, colors.$transparentColor, colors.$white 100%); | ||
width: 100%; | ||
height: 2vh; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific reason we're having this height in dependance to the viewport height, instead of just a static (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that if the window height is small, the blur should be smaller so that more of the content is visible. But looking back on that decision, I think it makes more sense to just always keep the blur a small pixel value, since it doesn't really ever need to be large |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import { useState } from "react"; | ||
import { useIntl } from "react-intl"; | ||
|
||
import { ResizablePanels } from "components/ui/ResizablePanels"; | ||
|
@@ -19,6 +20,12 @@ export const StreamTestingPanel: React.FC<unknown> = () => { | |
stream: selectedStream.name, | ||
config: configJson, | ||
}); | ||
const [logsFlex, setLogsFlex] = useState(0); | ||
|
||
const handleLogsTitleClick = () => { | ||
// expand to 50% if it is currently less than 50%, otherwise minimize it | ||
setLogsFlex((prevFlex) => (prevFlex < 0.5 ? 0.5 : 0)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nitpick: my personal feeling would be that we should always collapse it if it's somewhat open and open to 50% if it's completely closed. It feels a bit more aligned with what I personally would expect, but not having extremely strong feelings around that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would make sense too! I was going back and forth between that approach and the one I implemented, so your comment has convinced me |
||
}; | ||
|
||
return ( | ||
<div className={styles.container}> | ||
|
@@ -39,9 +46,14 @@ export const StreamTestingPanel: React.FC<unknown> = () => { | |
}} | ||
secondPanel={{ | ||
className: styles.logsContainer, | ||
children: <LogsDisplay logs={streamReadData.logs} />, | ||
children: <LogsDisplay logs={streamReadData.logs} onTitleClick={handleLogsTitleClick} />, | ||
minWidth: 30, | ||
flex: 0, | ||
flex: logsFlex, | ||
onStopResize: (newFlex) => { | ||
if (newFlex) { | ||
setLogsFlex(newFlex); | ||
} | ||
}, | ||
}} | ||
hideSecondPanel={streamReadData.logs.length === 0} | ||
/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
// Take in any, because that is what JSON.stringify() takes in | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
export function formatJson(json: any): string { | ||
lmossman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return JSON.stringify(json, null, 2); | ||
} |
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.
We have a
NumberBadge
component that should ideally be used here. (I know not directly relevant to this PR, but I just noticed it). We can extend that for more colors if needed.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 was not aware of that, thanks!