-
Notifications
You must be signed in to change notification settings - Fork 3.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(web): templates store modal workflow preview #3450
feat(web): templates store modal workflow preview #3450
Conversation
NV-2271 🏪 [Template store] Create detailed workflow preview (right side modal)
What?Template store/List of the templates/Description Why?Allow users to see extra information regarding a particular template before they make a decision regarding its usage of it.
How?Display the name of the template Display a description of the template Display a workflow of the template, it can be scrolled up and down Display a button Use template and Made by Novu sign and icon Workflow list - Template store (Figma) White theme |
@@ -16,7 +16,7 @@ import { parseUrl } from '../../../utils/routeUtils'; | |||
import { OnBoardingAnalyticsEnum } from '../../../pages/quick-start/consts'; | |||
import { useSegment } from '../../providers/SegmentProvider'; | |||
import { useDigestDemoFlowContext } from './DigestDemoFlowProvider'; | |||
import { NodeStep } from '../common'; | |||
import { NodeStep } from '../../workflow-nodes'; |
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.
moved to another folder, not specific to the quick-start
data: { label: string; email?: string }; | ||
Handlers: React.FC<any>; | ||
Icon: React.FC<any>; | ||
ActionItem?: React.ReactNode; | ||
ContentItem?: React.ReactNode; | ||
}; | ||
|
||
export function NodeStep({ Handlers, Icon, data, ActionItem, ContentItem }: NodeStepProps) { | ||
export function NodeStep({ className, Handlers, Icon, data, ActionItem, ContentItem }: NodeStepProps) { |
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.
allow to style it with className
and styled
const NodeStepStyled = styled(NodeStep)` | ||
width: 200px; | ||
`; |
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.
that's how easy is to update the styles
const STEPS: IFormStep[] = [ | ||
{ | ||
_id: 'digest', | ||
name: 'Digest', | ||
template: { | ||
type: StepTypeEnum.DIGEST, | ||
content: '', | ||
}, | ||
}, | ||
{ | ||
_id: 'email', | ||
name: 'Email', | ||
template: { | ||
name: 'Test email', | ||
subject: 'Test subject', | ||
title: 'Test title', | ||
type: StepTypeEnum.EMAIL, | ||
content: 'adsf', | ||
}, | ||
}, | ||
]; | ||
|
||
const STEPS2: IFormStep[] = [ | ||
{ | ||
_id: 'in-app', | ||
name: 'In-App', | ||
template: { | ||
type: StepTypeEnum.IN_APP, | ||
content: '', | ||
}, | ||
}, | ||
{ | ||
_id: 'digest', | ||
name: 'Digest', | ||
template: { | ||
type: StepTypeEnum.DIGEST, | ||
content: '', | ||
}, | ||
}, | ||
{ | ||
_id: 'email', | ||
name: 'Email', | ||
template: { | ||
name: 'Test email', | ||
type: StepTypeEnum.EMAIL, | ||
content: 'adsf', | ||
}, | ||
}, | ||
{ | ||
_id: 'push', | ||
name: 'Push', | ||
template: { | ||
type: StepTypeEnum.PUSH, | ||
content: 'Test push', | ||
}, | ||
}, | ||
]; |
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.
mock steps
<FlowEditor | ||
steps={selectedTemplate.steps} | ||
nodeTypes={nodeTypes} | ||
zoomOnScroll={false} | ||
zoomOnPinch={false} | ||
zoomOnDoubleClick={false} | ||
panOnDrag={false} | ||
panOnScroll | ||
preventScrolling={false} | ||
nodesDraggable={false} | ||
elementsSelectable={false} | ||
nodesConnectable={false} | ||
selectNodesOnDrag={false} | ||
wrapperStyles={{ | ||
height: '100%', | ||
width: '100%', | ||
minHeight: '300px', | ||
}} |
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.
reusing the same FlowEditor
component as we do use in the Template Editor, allowing to pass any react-flow
props
const onStepInit = async () => { | ||
await trigger('steps'); | ||
}; |
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.
same
const nodeTypes = { | ||
channelNode: ChannelNode, | ||
triggerNode: TriggerNode, | ||
addNode: AddNode, | ||
}; | ||
|
||
const edgeTypes = { special: AddNodeEdge }; |
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.
These were hardcoded before in the FlowEditor
and they are using a lot of different functionalities, like react-hook-form
etc., but it's only specific to the Template Editor. Now we can define whatever steps we want and we can reuse FlowEditor
.
const DEFAULT_WRAPPER_STYLES = { | ||
height: '100%', | ||
width: '100%', | ||
minHeight: '600px', | ||
}; |
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.
were defined on the first div wrapper before
interface IFlowEditorProps extends ReactFlowProps { | ||
steps: IFormStep[]; | ||
dragging?: boolean; | ||
errors?: any; | ||
nodeTypes: { | ||
triggerNode: ComponentType<NodeProps>; | ||
channelNode: ComponentType<NodeProps>; | ||
addNode?: ComponentType<NodeProps>; | ||
}; | ||
edgeTypes?: { special: ComponentType<EdgeProps> }; | ||
withControls?: boolean; | ||
wrapperStyles?: React.CSSProperties; | ||
onDelete?: (id: string) => void; | ||
onStepInit?: (step: IFormStep) => Promise<void>; | ||
addStep?: (channelType: StepTypeEnum, id: string, index?: number) => void; | ||
} |
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.
extends ReactFlowProps
and adds some other useful props
zoomOnScroll = false, | ||
preventScrolling = true, | ||
nodesConnectable = false, | ||
nodesDraggable = true, | ||
minZoom = 0.5, | ||
maxZoom = 1.5, | ||
defaultZoom = 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.
before these were defined in the const reactFlowDefaultProps
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.
Just a code bits review. Looks good overall! 🌟
setNodes((nds) => nds.concat(newNode)); | ||
|
||
const newEdge = buildNewEdge(parentId, newId, edgeTypes ? 'special' : '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.
The condition of the third argument is a bit hidden and can be missed for someone not familiar with code. I'd suggest to extract it so it is a bit more clear.
> | ||
<Controls /> | ||
{withControls && <Controls />} |
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.
Thinking if we should pass withControls
as a show
property to the Controls component and the proper component to handle their rendering rather than the parent. What do you 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.
This Controls
component is from the react-flow-renderer
library and it doesn't have the show
prop.
But still, I think it's better to do it on the parent level, otherwise, if you do it on the child level then you might register some hooks unnecessarily and the React will create a fiber for that component and will do all its logic aka reconciliation work.
return ( | ||
<> | ||
<Handle type="target" id="b" position={Position.Top} /> | ||
<Handle style={noChildStyle} type="source" id="a" position={Position.Bottom} /> |
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.
Would be a problem if multiple components share the same id? Even if being part of a parent component? Thinking in the reconciliation algorithm and when debugging.
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 if this is even needed, but I've copied it from the other place 🥲 I'll check it.
This id
is not used as the HTML el id
but as the internal library handleId
. It shouldn't have any impact on the reconciliation.
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 id
's are not needed, I'll remove them
What change does this PR introduce?
In this PR I've implemented the workflow preview in the Templates Store Modal.
The preview is using the same
FlowEditor
that we use in the Template Editor, so it's been a little refactored by moving the code that shouldn't be there outside of the component. I'll leave the explanation in the comments.Why was this change needed?
Templates Store PRD.
Other information (Screenshots)
Screen.Recording.2023-05-17.at.23.06.20.mov