-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Mark/update settings to toggle auto detect schema changes notifications #20395
Conversation
...nPage/pages/ConnectionItemPage/ConnectionReplicationTab/ConnectionReplicationTab.module.scss
Outdated
Show resolved
Hide resolved
...Page/pages/ConnectionItemPage/ConnectionSettingsTab/components/SchemaUpdateNotifications.tsx
Outdated
Show resolved
Hide resolved
...ctionItemPage/ConnectionTransformationTab/components/DbtCloudTransformationsCard.module.scss
Outdated
Show resolved
Hide resolved
...es/ConnectionItemPage/ConnectionTransformationTab/components/DbtCloudTransformationsCard.tsx
Outdated
Show resolved
Hide resolved
...ages/ConnectionPage/pages/ConnectionItemPage/ConnectionSettingsTab/ConnectionSettingsTab.tsx
Outdated
Show resolved
Hide resolved
...nPage/pages/ConnectionItemPage/ConnectionReplicationTab/ConnectionReplicationTab.module.scss
Outdated
Show resolved
Hide resolved
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.
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
.../ConnectionPage/pages/ConnectionItemPage/ConnectionStatusTab/ConnectionStatusTab.module.scss
Outdated
Show resolved
Hide resolved
.../ConnectionPage/pages/ConnectionItemPage/ConnectionStatusTab/ConnectionStatusTab.module.scss
Outdated
Show resolved
Hide resolved
...ctionItemPage/ConnectionTransformationTab/components/DbtCloudTransformationsCard.module.scss
Outdated
Show resolved
Hide resolved
...ctionItemPage/ConnectionTransformationTab/components/DbtCloudTransformationsCard.module.scss
Outdated
Show resolved
Hide resolved
Sorry for such a mess. I will return to the original (master branch) structure 😇 |
f49aa8c
to
6a697c4
Compare
airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/SchemaUpdateNotifications.tsx
Outdated
Show resolved
Hide resolved
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.
left one question to @krishnaglick besides that LGTM. Will approve, after all other comments from guys will be fixed
b49923a
to
e9b7458
Compare
After some investigation and a discussion with Mark:
I've made two tickets to extend off this: Edit: Correction, there was a slack conversation about the error toast. |
55cb035
to
076ac4d
Compare
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.
LGTM
- Refactored structure of ConnectionItemPage - Added toggle auto-detect schema changes notifications
076ac4d
to
9a70664
Compare
- Added toggle auto-detect schema changes notifications
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.
tested locally with flag off and on, creating and editing connections (did not have any connections from before this PR to test)
code lgtm
What
Closes #17830
How
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