-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(api): add ui scehma #6764
feat(api): add ui scehma #6764
Conversation
@novu/client
@novu/framework
@novu/js
@novu/headless
@novu/nextjs
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
commit: |
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -1,6 +1,5 @@ | |||
import { JSONSchema } from 'json-schema-to-ts'; | |||
|
|||
export type StepSchemaDto = { | |||
controls: JSONSchema; |
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.
As discussed, I've already removed the controls from the DTO in PR #6742.
|
||
export type UiSchema = { | ||
type: `${StepType}`; | ||
properties?: Properties; |
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.
Should properties always be supplied?
properties?: Properties; | |
properties: Properties; |
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.
Furthermore, I think it would be preferable to type out the Properties
interface separately rather than collect all the different ui schema properties into the type. That way the Properties
interface can evolve independently from the actual UI schema definitions.
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.
i agree. I was not sure what approach to take here: simpler or more generic. you mean to convert the type to a generic type instead. did I get you right?
subject: { type: 'inAppSubject' }, | ||
body: { type: 'inAppBody' }, | ||
avatar: { type: 'inAppAvatar' }, | ||
primaryAction: { type: 'inAppPrimaryAction' }, |
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.
I wanted to clarify the reasoning behind our approach. The goal was not to create a one-to-one mapping but rather to specify particular overrides for the component type. Having a complete mapping may not be necessary, as it requires the front-end to duplicate specific elements for each type of step, which can lead to unnecessary complexity.
Could we revisit this to ensure it aligns with our original objectives?
apps/api/src/app/step-schemas/usecases/get-step-schema/get-step-schema.usecase.ts
Outdated
Show resolved
Hide resolved
export enum UiControlGroupEnum { | ||
INBOX = 'INBOX', | ||
} | ||
export class UiSchema { | ||
controlGroup?: UiControlGroupEnum.INBOX; | ||
elements?: UiElement[]; | ||
} | ||
class UiElement { | ||
elementTypeOverride: UiElementTypeEnum; | ||
} | ||
|
||
enum UiElementTypeEnum { | ||
EMAIL_EDITOR = 'EMAIL_EDITOR', | ||
} |
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.
Should all of these be deleted with the new approach?
export enum UiControlGroupEnum { | |
INBOX = 'INBOX', | |
} | |
export class UiSchema { | |
controlGroup?: UiControlGroupEnum.INBOX; | |
elements?: UiElement[]; | |
} | |
class UiElement { | |
elementTypeOverride: UiElementTypeEnum; | |
} | |
enum UiElementTypeEnum { | |
EMAIL_EDITOR = 'EMAIL_EDITOR', | |
} |
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.
i will, i thought to delete it separately while removing the controls from the /workflows response
|
||
const pushProperties = { | ||
subject: { type: 'pushSubject' }, | ||
body: { type: 'pushBody' }, |
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.
why not just text? it's going to make the FE work super problematic
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.
as discussed in the meeting, in this solution, the client knows something
rather than nothing
the second solution suggested in the DX guide. Here, the client needs to know how to render pushBody
. In the future, we can expand this approach to provide even more data to the client if needed.
body: { type: 'pushBody' }, | ||
}; | ||
|
||
const smsProperties = { |
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.
also it's not correct, because in code first they can have as many controls as they want, they shouldn't be limited
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.
i think we still need support for code first, we will need to store uiSchema on sync in the database and then extract it here.
What changed? Why was the change needed?
removed control scehma from step schema endpoint.
added UI schema concept.
added first implementatino of ui schema for inApp with control group
https://linear.app/novu/issue/NV-4547/create-ui-schema
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer