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

Mark/update settings to toggle auto detect schema changes notifications #20395

Conversation

matter-q
Copy link
Contributor

@matter-q matter-q commented Dec 12, 2022

What

Closes #17830

How

  • Added new card to toggle auto-detect schema changes notifications

For test purposes, you need to use the feature variable: REACT_APP_FEATURE_ALLOW_AUTO_DETECT_SCHEMA_CHANGES=true

Loom

https://www.loom.com/share/4fc94c68c2894538b6b73c19e2fcc48a

@matter-q matter-q requested a review from a team as a code owner December 12, 2022 22:13
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 12, 2022
@krishnaglick krishnaglick self-requested a review December 13, 2022 14:33
@teallarson teallarson self-requested a review December 13, 2022 14:41
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

On the right track!

Please remove the file rearrangements and save for a separate PR. It makes the diff muddy and difficult to tell what has been changed in the code.

A few other notes:

  • we should not be using relative imports for our root stylesheets
  • this entire component needs to be hidden if the auto-detect schema changes feature is not turned on

@matter-q
Copy link
Contributor Author

On the right track!

Please remove the file rearrangements and save for a separate PR. It makes the diff muddy and difficult to tell what has been changed in the code.

A few other notes:

  • we should not be using relative imports for our root stylesheets
  • this entire component needs to be hidden if the auto-detect schema changes feature is not turned on

Sorry for such a mess. I will return to the original (master branch) structure 😇

@matter-q matter-q force-pushed the mark/update-settings-to-toggle-auto-detect-schema-changes-notifications branch 2 times, most recently from f49aa8c to 6a697c4 Compare December 13, 2022 19:51
Copy link
Contributor

@YatsukBogdan1 YatsukBogdan1 left a comment

Choose a reason for hiding this comment

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

left one question to @krishnaglick besides that LGTM. Will approve, after all other comments from guys will be fixed

@matter-q matter-q force-pushed the mark/update-settings-to-toggle-auto-detect-schema-changes-notifications branch from b49923a to e9b7458 Compare December 14, 2022 21:21
@krishnaglick
Copy link
Contributor

krishnaglick commented Dec 15, 2022

After some investigation and a discussion with Mark:

  1. Pull out the local loading state and use the loading state from the connection edit service.
  2. Remove the error notification toast.
    This will slim down the logic in the component to just the update call, and utilization of loading state.

I've made two tickets to extend off this:
Use local loading
Toast errors

Edit: Correction, there was a slack conversation about the error toast.

@matter-q matter-q force-pushed the mark/update-settings-to-toggle-auto-detect-schema-changes-notifications branch from 55cb035 to 076ac4d Compare December 15, 2022 21:09
Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

LGTM

- Refactored structure of ConnectionItemPage
- Added toggle auto-detect schema changes notifications
@matter-q matter-q force-pushed the mark/update-settings-to-toggle-auto-detect-schema-changes-notifications branch from 076ac4d to 9a70664 Compare December 16, 2022 19:50
- Added toggle auto-detect schema changes notifications
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

tested locally with flag off and on, creating and editing connections (did not have any connections from before this PR to test)

code lgtm

@matter-q matter-q merged commit e92b47f into master Dec 16, 2022
@matter-q matter-q deleted the mark/update-settings-to-toggle-auto-detect-schema-changes-notifications branch December 16, 2022 20:34
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.

Update settings to toggle auto-detect schema changes notifications
6 participants