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

Nv 2232 to in app notification center provider #3427

Merged
merged 54 commits into from
May 26, 2023

Conversation

davidsoderberg
Copy link
Contributor

No description provided.

@linear
Copy link

linear bot commented May 16, 2023

NV-2232 🔔 to 📡 In-App Notification center provider on integration store

Why? (Context)

Currently, the notification center configuration is scattered across multiple pages including the settings page. This creates for us multiple problems when it comes to understanding if a user has created a notification center integration and etc…

What?

  • Create an In-App channel integration in the integration store
  • Treat novu in-app as any other provider in the system
  • Move the notification center settings to the provider modal
  • On-boarding discoverability: Show the user how he can integrate Novu using: Iframe, In-App Widget

Definition of Done

  • The current in-app configuration should be moved to the provider entity (either migration or any other way)
  • The user should be able to create and enable an in-app integration

Solution

Loom solution guide

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

I've found a few things, but I haven't checked the integration logic. I'll do the rest of the testing later.

  1. When I do create a new org I see this error.
    Screenshot 2023-05-25 at 12 35 21
    Screenshot 2023-05-25 at 12 36 17

  2. We should remove the body scrollbars when the modal is opened.
    Screenshot 2023-05-25 at 11 02 44

  3. I don't see in the design "Configure Later" button
    Screenshot 2023-05-25 at 11 07 05

  4. Web Component tile is a little bigger that the others.
    Screenshot 2023-05-25 at 11 39 48

@davidsoderberg
Copy link
Contributor Author

davidsoderberg commented May 25, 2023

I've found a few things, but I haven't checked the integration logic. I'll do the rest of the testing later.

  1. When I do create a new org I see this error.
    Screenshot 2023-05-25 at 12 35 21
    Screenshot 2023-05-25 at 12 36 17
  2. We should remove the body scrollbars when the modal is opened.
    Screenshot 2023-05-25 at 11 02 44
  3. I don't see in the design "Configure Later" button
    Screenshot 2023-05-25 at 11 07 05
  4. Web Component tile is a little bigger that the others.
    Screenshot 2023-05-25 at 11 39 48

All of these are now resolved. 3 was added to keep it aligned with the quickstart guide.

Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

Great work. Mostly light touch comments. 👏🏻


export async function createInAppIntegration() {
// eslint-disable-next-line no-console
console.log('start migration - in app integration');
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace the console.logs for Logger from @nestjs/common so we can also track the migrations with out telemetry tools. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have not done it in migrations this far, but maybe it is a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to change the file name and path, right? Still need help with 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.

Yes lets try to look at that tomorrow

apps/web/cypress/tests/changes.spec.ts Show resolved Hide resolved
apps/web/src/pages/quick-start/components/SetupStatus.tsx Outdated Show resolved Hide resolved
@davidsoderberg davidsoderberg added this pull request to the merge queue May 26, 2023
Merged via the queue into next with commit d14fdd7 May 26, 2023
33 checks passed
@davidsoderberg davidsoderberg deleted the nv-2232-to-in-app-notification-center-provider branch May 26, 2023 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants