Skip to content
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

Merged
merged 12 commits into from
Nov 10, 2022

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Nov 9, 2022

What

This change comes from a suggestion on an earlier PR, and it allows users to click on the Connector logs title in the logs drawer of the Connector Builder to expand the log drawer to a 50% size (and click it again to minimize it).

Screen recording demonstrating this:

Screen.Recording.2022-11-08.at.6.04.57.PM.mov

How

  • Turns the Connector logs title into a clickable button, that either expands the log view to 50% size, or minimizes it if it is already >= 50% size
  • Adds a small fade to the bottom of the result display, so that the text does not look clipped when the logs drawer is resized
  • Memoizes the logs and results JSON formatting output, to improve performance on re-rendering, as suggested by this other comment: 🪟 🎉 Add connector builder logs viewer and improve page/slice state #18949 (comment)

@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Nov 9, 2022
@lmossman lmossman changed the title add ability to click on logs to expand them [Connector Builder] Add ability to click on logs to expand them Nov 9, 2022
@lmossman lmossman requested a review from timroes November 9, 2022 01:54
@lmossman lmossman marked this pull request as ready for review November 9, 2022 01:54
@lmossman lmossman requested a review from a team as a code owner November 9, 2022 01:54
Comment on lines 25 to 27
<Text className={styles.numLogsDisplay} size="xs" bold>
{logs.length}
</Text>
Copy link
Collaborator

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.

Copy link
Contributor Author

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!

.tabPanel::after {
content: "";
position: absolute;
z-index: 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 _z-indices.scss utility. Given that the purpose here is only to lift it slightly above the content, i.e. doing a "+1" I feel we can keep this in place here.

pointer-events: none;
background-image: linear-gradient(to bottom, colors.$transparentColor, colors.$white 100%);
width: 100%;
height: 2vh;
Copy link
Collaborator

Choose a reason for hiding this comment

The 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. rem or px) height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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


const handleLogsTitleClick = () => {
// expand to 50% if it is currently less than 50%, otherwise minimize it
setLogsFlex((prevFlex) => (prevFlex < 0.5 ? 0.5 : 0));
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tested locally in Chrome Linux, left some minor code style comments and a usability comment, but nothing of it would be blocking for me.

akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* add ability to click on logs to expand them

* memoize the result and log formatting

* set flex in a controlled way

* fix flex sizing of title

* use NumberBadge

* use static height for fade

* Update airbyte-webapp/src/components/StreamTestingPanel/utils.ts

Co-authored-by: Tim Roes <[email protected]>

* Update airbyte-webapp/src/components/ui/ResizablePanels/ResizablePanels.tsx

Co-authored-by: Tim Roes <[email protected]>

* Update airbyte-webapp/src/components/ui/ResizablePanels/ResizablePanels.tsx

Co-authored-by: Tim Roes <[email protected]>

* remove comment

* fix behavior of title click

Co-authored-by: Tim Roes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants