-
Notifications
You must be signed in to change notification settings - Fork 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
feat(dashboard): in-app editor form driven by BE schema #6877
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -9,33 +9,36 @@ const redirectSchema = { | |||
url: { | |||
type: 'string', | |||
pattern: ABSOLUTE_AND_RELATIVE_URL_REGEX, | |||
default: '', |
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.
Added the default values to the dataSchema
, these are used to generate the default values for the form. The default values are required for all visible text fields by the react-hook-form
, ref.
@@ -75,6 +75,7 @@ | |||
"devDependencies": { | |||
"@clerk/types": "^4.6.1", | |||
"@eslint/js": "^9.9.0", | |||
"@hookform/devtools": "^4.3.0", |
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.
A really useful tool to debug the react-hook-form
state
{error && ( | ||
<RiErrorWarningFill className="text-destructive outline-destructive absolute right-0 top-0 size-3 -translate-y-1/2 translate-x-1/2 rounded-full outline outline-1 outline-offset-1" /> | ||
)} |
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.
it was missing the error icon
({ className, children, ...props }, ref) => { | ||
const { error, formMessageId } = useFormField(); | ||
const body = error ? String(error?.message) : children; | ||
const FormMessagePure = React.forwardRef< |
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.
The FormMessagePure
is a label + icon that is not dependent on the FormField
, it's used in a few cases.
@@ -78,7 +78,7 @@ const Step = () => { | |||
const { stepType: channel } = useStep(); | |||
switch (channel) { | |||
case StepTypeEnum.IN_APP: | |||
return <InApp />; | |||
return <ConfigureInApp />; |
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.
renamed
const primaryActionKey = 'primaryAction'; | ||
const secondaryActionKey = 'secondaryAction'; | ||
|
||
export const InAppEditor = ({ uiSchema }: { uiSchema?: UiSchema }) => { |
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.
The component that renders the In-App Editor UI form dynamically based on the uiSchema
.
|
||
const tabsContentClassName = 'h-full w-full px-3 py-3.5'; | ||
|
||
export const InAppTabs = ({ workflow, step }: { workflow: WorkflowResponseDto; step: StepDataDto }) => { |
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.
This is the main component that is rendered in the In-App drawer, it consists of the editor and preview tabs, react-hook-form and submission logic.
withHint?: boolean; | ||
} & Pick<InputFieldProps, 'size'>; | ||
|
||
export const URLInput = ({ |
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've moved this component from the primitives as it's based on the form fields, and I couldn't make it work without the form fields.
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.
Makes sense. Should we create an elements
directory for components like this? If it makes sense outside of the workflow editor of course.
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.
Let's see if there will be a need to reuse this one.
* The function will recursively build the schema based on the JSONSchema object. | ||
* It removes empty strings and objects with empty required fields during the transformation phase after parsing. | ||
*/ | ||
export const buildDynamicZodSchema = (obj: JSONSchema): z.AnyZodObject => { |
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.
This is the util used to parse the dataSchema
(which is JSONSchema type) to Zod.
The converted schema is used in the form for the validation and then transformation after the parsing (success validation) phase. The transformed value is returned in the onSubmit
handler.
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.
if you need zod why do we serve jsonSchema? @SokratisVidros
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 guess that the FE prefers to work with Zod for client-side validations so they transform the JSON schema to zod.
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.
@tatarco, the Zod schema is not serializable; please check this discussion. There are tools that can convert Zod <> JSON schema and vice versa, but they are not "perfect" and require some runtime evaluation.
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.
@tatarco is right. We should use the JSON Schema binding for React-hook form rather than transform to a Zod schema, else we risk loss of validation strictness.
apps/dashboard/src/utils/schema.ts
Outdated
/** | ||
* Build default values based on the JSONSchema object. | ||
*/ | ||
export const buildDefaultValues = (obj: JSONSchema): object => { |
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.
This util takes the dataSchema: JSONSchema
and retrieves the default values from it.
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.
@SokratisVidros the work done here shows how unnecessary was using the json schema if we need to manually parse it in the FE, we should have made the zod available via shared library and convert the zod to jsonschema for the library usage
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 am not sure I understand the comment. If we hardcode the values per step how can we support dynamic configurations?
@novu/client
@novu/headless
@novu/framework
@novu/js
@novu/nextjs
@novu/nest
@novu/node
@novu/notification-center
novu
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
apps/dashboard/src/components/primitives/form/avatar-picker.tsx
Outdated
Show resolved
Hide resolved
<AvatarImage src={props.value as string} /> | ||
</Avatar> | ||
) : ( | ||
<RiImageEditFill className="size-5" /> | ||
)} | ||
{error && ( | ||
<RiErrorWarningFill className="text-destructive outline-destructive absolute right-0 top-0 size-3 -translate-y-1/2 translate-x-1/2 rounded-full outline outline-1 outline-offset-1" /> |
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.
You can do m-auto inset-0
to center this a bit cleaner i think.
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'm not sure I get it, we want to show the error icon in the top right corner of the avatar. Can you please clarify?
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.
ah i thought you were centering. I think that you can do inset: 0 0 auto auto;
apps/dashboard/src/components/workflow-editor/steps/in-app/in-app-redirect.tsx
Outdated
Show resolved
Hide resolved
withHint?: boolean; | ||
} & Pick<InputFieldProps, 'size'>; | ||
|
||
export const URLInput = ({ |
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.
Makes sense. Should we create an elements
directory for components like this? If it makes sense outside of the workflow editor of course.
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.
Wow. Is there like no easier way to do this ?
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.
There is a json-schema-to-zod lib, but it generates the string JS module that you have to evaluate at runtime, and there are some limitations.
Also this custom code has additional "transformations" that are applied on the form object after the "succesfull validation" and will be returned in the "onSuccess" handler of the form.
</div> | ||
{body && getComponentByType({ component: body.component })} | ||
{(primaryAction || secondaryAction) && | ||
getComponentByType({ |
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.
doesn't it means that if the secondary key is missing it will still show both options in the UI ? same for primary
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.
Hmmm, also when there is no action set yet, the widget should be displayed.
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.
doesn't it means that if the secondary key is missing it will still show both options in the UI ? same for primary
@tatarco No, this code is just responsible for rendering or not the "whole" component, but it has the logic inside that determines which buttons to render or not.
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.
Hmmm, also when there is no action set yet, the widget should be displayed.
@SokratisVidros primaryAction || secondaryAction
variables are taken from the controls.uiSchema
, which controls what components to render. But the controls.dataSchema
default values or controls.values
do control the "state" of the component, meaning that when there are no control.values
, the component will still be rendered with a "no action" state.
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.
you can see that in the attached video ;)
apps/dashboard/src/components/workflow-editor/steps/in-app/in-app-editor.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/components/workflow-editor/steps/in-app/in-app-editor.tsx
Outdated
Show resolved
Hide resolved
</div> | ||
{body && getComponentByType({ component: body.component })} | ||
{(primaryAction || secondaryAction) && | ||
getComponentByType({ |
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.
Hmmm, also when there is no action set yet, the widget should be displayed.
* The function will recursively build the schema based on the JSONSchema object. | ||
* It removes empty strings and objects with empty required fields during the transformation phase after parsing. | ||
*/ | ||
export const buildDynamicZodSchema = (obj: JSONSchema): z.AnyZodObject => { |
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 guess that the FE prefers to work with Zod for client-side validations so they transform the JSON schema to zod.
apps/dashboard/src/utils/schema.ts
Outdated
/** | ||
* Build default values based on the JSONSchema object. | ||
*/ | ||
export const buildDefaultValues = (obj: JSONSchema): object => { |
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 am not sure I understand the comment. If we hardcode the values per step how can we support dynamic configurations?
What changed? Why was the change needed?
Dashboard - In-App Editor - BE schema-driven form generation and validation.
In this PR:
dataSchema
returned from BEreact-hook-form
uiSchema
and component typesScreenshots
Screen.Recording.2024-11-06.at.23.49.51.mov