Mark/update notification settings design #18159

matter-q · 2022-10-19T13:38:36Z

What

Closes #17087

How

Notification settings design page redesign

Moved the Save changes button under the card
Changed the behavior of the notification form
Added button Need help with Webhook URL?
Created notification guide panel

Figma design

Loom

Updated Loom with the latest changes:
https://www.loom.com/share/b8ecd9f3a3b04a7f8c5a93fc696f80d2

edmundito

The code is looking really good. I did some local testing and there are a couple of areas that can be improved in terms of indicating success and error. The rest is mostly style issues.

Additionally, we are trying to move away from using the public folder (#15956) and instead the webhook-guide.png image should be alongside the webhook component.

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.module.scss

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

edmundito · 2022-10-20T13:30:18Z

airbyte-webapp/src/locales/en.json

  "settings.metrics": "Metrics",
-  "settings.notificationSettings": "Notification Settings",
+  "settings.notificationsParameters": "Notifications parameters",


@andyjih This change is for the title of the Notifications page to follow what was exactly in the Figma. What should it be here? Notification Settings, Notifications, or Notification parameters?

@andyjih Could you also provide messages for the Test action button when it's a success and failed operation?

My question is covered by Nicolas Philippot

Decided on Notification Settings during standup

edmundito · 2022-10-20T14:07:39Z

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

+  const webhookChange = async (action: WebhookAction, data: WebhookPayload) => {
+    setFormAction((value) => ({ ...value, [action]: true }));
+    if (action === WebhookAction.Test) {
+      await testWebhook(data);


There should be a feedback message when the notification test succeeds or fails, the same for trying to save the connection. There is no feedback message, even on the main branch. Would it be possible to fix this here?

Do we have a specific service or component for notifications? Like we have a message when the connection with the server is unavailable (it's a small toast). The previous implementation was not good and not standard for all such forms.

Agree with @edmundito. Also tested locally, and we indeed need to show the user the webhook URL testing (was it successful or not)

@matter-q yeah, we have such a service -

airbyte/airbyte-webapp/src/hooks/services/Notification/NotificationService.tsx

Line 12 in 9db329e

const NotificationService = ({ children }: { children: React.ReactNode }) => {

Please note, there can be 2 possible cases when a testing hook request can be failed:

4xx status response from BE (if we have sent the totally wrong URL)

2xx status response from 3rd party hook service (need to check the response)

I've used the https://webhook.site/ service for testing webhooks

After a conversation with Nicolas Philippot, we decided to handle only the error message on the Test button. The error message should be below the input.

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

dizel852

Overall - code LGTM.
Was reviewing after @edmundito comment fixes.

Left a comment regarding NoficationsService.

airbyte-webapp/src/components/icons/PlayIcon.tsx

airbyte-webapp/src/utils/links.ts

edmundito

Had some notes about the error handling

airbyte-webapp/src/pages/SettingsPage/pages/NotificationPage/components/WebHookForm.tsx

dizel852

Code LGTM, tested locally.
Let's wait for final approval from @edmundito

@edmundito question:
Currently, we show the input error in case the test is failed.
Why we decided do not to show the message in case of successful testing?

matter-q · 2022-10-28T15:01:55Z

Waiting for final UI decision from Niko 😀

dizel852 · 2022-10-28T15:19:54Z

@Upmitt

- Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

edmundito

Tested locally and testing/save/validation rules work as expected!
Tested links on doc and both navigate to the correct page

* master: (59 commits) 🪟🔧 Remove styled components (round 1) (#18766) Bump Airbyte version from 0.40.17 to 0.40.18 (#18827) ci: add job and run id to test reports (#18832) hide ConfigPersistence inside ConfigRepository to discourage use (#18803) Remove the bulk actions from ConfigPersistence (#18800) remove config persistence from seeding logic (#18749) Remove ConfigPersistence usage from SecretsMigrator (#18747) Add normalization changelog and bump normalization version in platform (#18813) 🐛Source Exchange Rates: Fix handling error during check connection (#18726) 🐛Destination Google Sheets: Fix empty headers list (#18729) Bump helm chart version reference to 0.40.40 (#18815) Use equalsIgnoreCase (#18810) [charts/airbyte-cron] Cleanup env vars (#18787) 🐛 Source Facebook Marketing: reduce request limit after specific error (#18734) Parameterize test_empty_streams and test_stream_with_1_airbyte_column by destination (#18197) Correct coinmarket spec (#18790) ci: use custom test-reporter action to upload job results (#18004) Fix unit tests in source relational db (#18789) query to include data plane attributes (#18531) Mark/update notification settings design (#18159) ...

* Update Notification Settings design - Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Oct 19, 2022

matter-q added the team/platform-move label Oct 19, 2022

matter-q marked this pull request as ready for review October 19, 2022 13:49

matter-q requested a review from a team as a code owner October 19, 2022 13:49

matter-q force-pushed the mark/update-notification-settings-design branch from fbe8ac6 to 5cbffc3 Compare October 19, 2022 13:52

edmundito suggested changes Oct 20, 2022

View reviewed changes

dizel852 requested changes Oct 21, 2022

View reviewed changes

matter-q force-pushed the mark/update-notification-settings-design branch 2 times, most recently from 55a370d to fe8e521 Compare October 25, 2022 14:28

matter-q requested review from edmundito and dizel852 October 25, 2022 15:39

edmundito reviewed Oct 25, 2022

View reviewed changes

airbyte-webapp/src/components/icons/PlayIcon.tsx Outdated Show resolved Hide resolved

airbyte-webapp/src/utils/links.ts Outdated Show resolved Hide resolved

edmundito reviewed Oct 25, 2022

View reviewed changes

matter-q force-pushed the mark/update-notification-settings-design branch from c15e5bd to 561010a Compare October 25, 2022 21:50

dizel852 approved these changes Oct 26, 2022

View reviewed changes

matter-q marked this pull request as draft October 28, 2022 10:18

matter-q changed the title ~~Mark/update notification settings design~~ [WIP] Mark/update notification settings design Oct 28, 2022

matter-q force-pushed the mark/update-notification-settings-design branch from 8ea279b to a519116 Compare October 28, 2022 16:36

matter-q changed the title ~~[WIP] Mark/update notification settings design~~ Mark/update notification settings design Nov 1, 2022

matter-q marked this pull request as ready for review November 1, 2022 01:10

matter-q added 4 commits November 1, 2022 03:11

Update Notification Settings design

d859dbc

- Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

Update Notification Settings design

d9adef8

- Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

Update Notification Settings design

6e27675

- Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

Update Notification Settings design

40fe886

- Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

matter-q force-pushed the mark/update-notification-settings-design branch from 555973d to 40fe886 Compare November 1, 2022 01:11

edmundito approved these changes Nov 1, 2022

View reviewed changes

matter-q merged commit e7ae961 into master Nov 1, 2022

matter-q deleted the mark/update-notification-settings-design branch November 1, 2022 17:26

octavia-squidington-iii mentioned this pull request Nov 1, 2022

Bump Airbyte version from 0.40.17 to 0.40.18 #18827

Merged

natalyjazzviolin pushed a commit that referenced this pull request Nov 3, 2022

Mark/update notification settings design (#18159)

0b3e8cd

* Update Notification Settings design - Removed styled-components - Added new behavior 'Test' and 'Submit' buttons

nataliekwong mentioned this pull request Jan 2, 2023

Improve usability of setting up webhook notifications #12518

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark/update notification settings design #18159

Mark/update notification settings design #18159

matter-q commented Oct 19, 2022 •

edited

Loading

edmundito left a comment

edmundito Oct 20, 2022

matter-q Oct 24, 2022 •

edited

Loading

matter-q Oct 25, 2022

teallarson Oct 25, 2022

edmundito Oct 20, 2022

matter-q Oct 20, 2022

dizel852 Oct 21, 2022

matter-q Oct 25, 2022

dizel852 left a comment

edmundito left a comment

dizel852 left a comment

matter-q commented Oct 28, 2022

dizel852 commented Oct 28, 2022

edmundito left a comment

Mark/update notification settings design #18159

Mark/update notification settings design #18159

Conversation

matter-q commented Oct 19, 2022 • edited Loading

What

How

Loom

edmundito left a comment

Choose a reason for hiding this comment

edmundito Oct 20, 2022

Choose a reason for hiding this comment

matter-q Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

matter-q Oct 25, 2022

Choose a reason for hiding this comment

teallarson Oct 25, 2022

Choose a reason for hiding this comment

edmundito Oct 20, 2022

Choose a reason for hiding this comment

matter-q Oct 20, 2022

Choose a reason for hiding this comment

dizel852 Oct 21, 2022

Choose a reason for hiding this comment

matter-q Oct 25, 2022

Choose a reason for hiding this comment

dizel852 left a comment

Choose a reason for hiding this comment

edmundito left a comment

Choose a reason for hiding this comment

dizel852 left a comment

Choose a reason for hiding this comment

matter-q commented Oct 28, 2022

dizel852 commented Oct 28, 2022

edmundito left a comment

Choose a reason for hiding this comment

matter-q commented Oct 19, 2022 •

edited

Loading

matter-q Oct 24, 2022 •

edited

Loading