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 form: Improve logs look and feel #20951

Merged
merged 10 commits into from
Jan 9, 2023

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jan 2, 2023

Fixes #20238

What

Improves the look of the logs in the connector form.

Old:
Screenshot 2022-12-08 at 17 19 15

New:
Screenshot 2023-01-02 at 17 14 47
Screenshot 2023-01-02 at 17 14 52

How

  • Remove the JobItem wrapper and use JobLogs directly - as there is only ever a single entry it doesn't make sense to wrap it into an accordion which looks like a list entry
  • Use a button to toggle logs visibility (not tied to advanced mode anymore)

Drive-by fix

In the old implementation there was a weird edge case:

  • Go to an existing connector configuration
  • Change the config so it will fail and hit "Save changes and test"
  • Tests are running and showing an error as expected
  • Click "Retest source" which tests the saved state of the connector
  • Test succeeds, but the log is still showing the result of the failed connection test with the local changes

This PR is clearing out the log when the connector is re-tested

Also fixes some inconsistent spacing between components. The whole connector form controls should be cleaned up, but that will be a separate PR.

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Jan 2, 2023
@flash1293 flash1293 marked this pull request as ready for review January 3, 2023 09:15
@flash1293 flash1293 requested a review from a team as a code owner January 3, 2023 09:15
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Had a few comments but nothing majorly blocking. Lmk if you want me to take another look if you decide to replace the button.

Tested locally and it works as expected

<Button
variant="secondary"
onClick={() => {
setLogsVisible(!logsVisible);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
setLogsVisible(!logsVisible);
setLogsVisible((prevLogsVisible) => !prevLogsVisible);

{job && (advancedMode || !job.succeeded) && <JobItem job={job} />}
{job && (
<div className={styles.connectionTestLogs}>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels like it may make more sense to use an expandable section here rather than a Button, similar to how the BuilderOptional component looks.

Since this button is just showing/hiding additional info, using the faAngleRight / faAngleDown icon next to some text that just says Connection test logs feels like a more natural UX. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, changed the button to look like this:
Screenshot 2023-01-06 at 13 32 25
Screenshot 2023-01-06 at 13 32 28

@flash1293 flash1293 enabled auto-merge (squash) January 9, 2023 10:09
@flash1293 flash1293 merged commit f12262c into master Jan 9, 2023
@flash1293 flash1293 deleted the flash1293/improve-logs-view branch January 9, 2023 10:30
jbfbell pushed a commit that referenced this pull request Jan 13, 2023
* improve logs look and feel in connector form

* add button to toggle

* lokk and feel
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.

Connector form: Improve logs rendering on failing connection test
3 participants