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

feat(wip): add in app activated support on the web #2703

Merged
merged 11 commits into from
Feb 17, 2023

Conversation

djabarovgeorge
Copy link
Contributor

What change does this PR introduce?

adds the new in-app onboarding.

Why was this change needed?

in order to provide the user more information about the implementation of the in-app channel.

Other information (Screenshots)

@linear
Copy link

linear bot commented Feb 8, 2023

Base automatically changed from NV-1643/in-app-set-up-support to in-app-onboarding February 12, 2023 12:44
Comment on lines 56 to 60
content:
'Welcome to Novu! Click on this notification to <b>visit the cloud admin panel</b> managing this message',
cta: {
type: ChannelCTATypeEnum.REDIRECT,
data: { url: `/templates/edit/${notificationTemplate?._id}` },
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 need to validate the template copy and if we want to reuse the CLI one or not.

@@ -0,0 +1,63 @@
export function localNavigate() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

created this module in order to manage the route location

@@ -0,0 +1,24 @@
import { useEffect, useState } from 'react';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

need to move to hooks folder

>
{embedCode}
</Prism>
<Prism code={embedCode} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related to the project (created reusable Prism)

Comment on lines 17 to 19
if (variable.name === 'subscriberId' && variable.manual) {
return variable.value;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment the subscriberId is set through the web client user, i added additional logic in order to pass manual param, so we can reuse this component (trigger button+modal)

Comment on lines +3 to +5
export function getNotificationGroups() {
return api.get(`/v1/notification-groups`);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related to the project moved it to the relevant api folder.

Comment on lines +4 to +6
export function getNotificationsList(page = 0, limit = 10) {
return api.getFullResponse(`/v1/notification-templates`, { page, limit });
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not related to the project moved it to the relevant api folder.

import { useQuery } from '@tanstack/react-query';
import { getNotificationGroups } from '../notification-groups';

export function useNotificationGroup() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

created useNotificationGroup in order to reuse it.

Comment on lines 243 to 256
cells={[
{
imagePath: `/static/images/frameworks/dark/react.png`,
navigateTo: '/quickstart/notification-center/set-up/react',
},
{
imagePath: `/static/images/frameworks/dark/angular.png`,
navigateTo: '/quickstart/notification-center/set-up/angular',
},
{
imagePath: `/static/images/frameworks/dark/vue.png`,
navigateTo: '/quickstart/notification-center/set-up/vue',
},
]}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it will be a good idea to refactor this one, move the cells param to consts file or create a component that will contain this logic (+ moving cells to consts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same for other routes.

}
/>
<Route
path="/quickstart/notification-center/trigger"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps we can rename i to a better one, on this page we trigger event + pointing the user to the templates page

Comment on lines 284 to 287
title={'Now is the time to trigger notification'}
description={'do it! '}
>
<TestNotificationTrigger />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this copy and TestNotificationTrigger is still wip.

@djabarovgeorge djabarovgeorge marked this pull request as ready for review February 12, 2023 13:03
Comment on lines +63 to +68
type: StepTypeEnum.IN_APP,
content:
'Welcome to Novu! Click on this notification to <b>visit the cloud admin panel</b> managing this message',
cta: {
type: ChannelCTATypeEnum.REDIRECT,
data: { url: `/templates/edit/${notificationTemplate?._id}` },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to think about the copy of the notification that will be sent to the user's bell

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we redirect the user back to the screen of the onboarding once clicked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can but only if we add to all of the clients (and by clients I mean react/angular/vue/demo and the embed code snippets) onNotificationClick handler with window.location.href update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scopsy let me know what you think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scopsy i will merge the branch, but let me know what you think, maybe we can remove it.

apps/api/src/app/integrations/integrations.controller.ts Outdated Show resolved Hide resolved
apps/web/src/pages/quick-start/steps/FrameworkSetup.tsx Outdated Show resolved Hide resolved
Comment on lines +63 to +68
type: StepTypeEnum.IN_APP,
content:
'Welcome to Novu! Click on this notification to <b>visit the cloud admin panel</b> managing this message',
cta: {
type: ChannelCTATypeEnum.REDIRECT,
data: { url: `/templates/edit/${notificationTemplate?._id}` },
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we redirect the user back to the screen of the onboarding once clicked?

apps/web/src/pages/quick-start/steps/Trigger.tsx Outdated Show resolved Hide resolved
@djabarovgeorge djabarovgeorge changed the base branch from in-app-onboarding to next February 16, 2023 12:38
@djabarovgeorge djabarovgeorge added this pull request to the merge queue Feb 17, 2023
Merged via the queue into next with commit 4dc2cbc Feb 17, 2023
@djabarovgeorge djabarovgeorge deleted the NV-1644/in-app-onboarding-on-the-web branch February 17, 2023 13:31
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.

2 participants