-
Notifications
You must be signed in to change notification settings - Fork 4.1k
feat(dashboard): email editor variable suggestions dropdown, pill, editing #7625
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
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
apps/dashboard/package.json
Outdated
@@ -23,7 +23,7 @@ | |||
"@codemirror/autocomplete": "^6.18.3", | |||
"@hookform/resolvers": "^3.9.0", | |||
"@lezer/highlight": "^1.2.1", | |||
"@maily-to/core": "^0.0.22", | |||
"@maily-to/core": "file:./maily-to-core-0.0.26.tgz", |
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.
temporarily until the arikchakma/maily.to#121 is merged
hoveredOptionIndex: number; | ||
}; | ||
|
||
const VariablesList = React.forwardRef<HTMLUListElement, VariablesListProps>( |
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 a reusable component, which is used later as the Email editor suggestions dropdown for variables
@@ -130,53 +64,6 @@ export const VariableSelect = ({ | |||
} | |||
}; | |||
|
|||
const scrollToOption = (index: number) => { |
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.
scrollToOption, next, prev
moved to reusable hook that manages the keyboard navigation useListKeyboardNavigation
const [hoveredOptionIndex, setHoveredOptionIndex] = useState(0); | ||
const variablesListRef = useRef<HTMLUListElement>(null); | ||
|
||
const { variablesListRef, hoveredOptionIndex, next, prev, reset, focusFirst } = useListKeyboardNavigation({ |
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.
TODO: better name
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.
Do we need a a separate hook for this? Since variables-list is a reusable component I'd expect the hook to live in there.
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.
yes we need it as it allows to control the list from the parent... for example MailyVariablesList
has the useImperativeHandle
with onKeyDown
which is called by it's parent
@@ -117,21 +116,18 @@ export function ControlInput({ | |||
value={value} | |||
onChange={onChange} | |||
/> | |||
<Popover open={!!selectedVariable} onOpenChange={handleOpenChange}> |
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 reusable component that later is used in the Email editor when editing the variable
useImperativeHandle(ref, () => ({ | ||
onKeyDown: ({ event }: { event: KeyboardEvent }) => { | ||
if (event.key === 'ArrowUp') { | ||
event.preventDefault(); | ||
prev(); | ||
return true; | ||
} | ||
|
||
if (event.key === 'ArrowDown') { | ||
event.preventDefault(); | ||
next(); | ||
return true; | ||
} | ||
|
||
if (event.key === 'Enter') { | ||
onSelect(options[hoveredOptionIndex].value ?? ''); | ||
reset(); | ||
return true; | ||
} | ||
|
||
return false; | ||
}, | ||
})); |
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 onKeyDown
is called by Maily core which is used by ref passed.
import { VARIABLE_REGEX_STRING } from '@/components/primitives/control-input/variable-plugin'; | ||
import { VariablePill } from '@/components/variable/variable-pill'; | ||
|
||
export function VariableView({ node, updateAttributes, editor }: NodeViewProps) { |
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 component represents the variable pill plus the editing popover. It's used as a Maily variable extension view.
@@ -39,6 +46,85 @@ export const Maily = ({ value, onChange, className, ...rest }: MailyProps) => { | |||
const isForBlockEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_ND_EMAIL_FOR_BLOCK_ENABLED); | |||
const isShowEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_ND_EMAIL_SHOW_ENABLED); | |||
|
|||
const calculateVariables = useCallback( |
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 from the Maily prop and reused in the variable extension
VariableExtension.extend({ | ||
addNodeView() { | ||
return ReactNodeViewRenderer(VariableView, { | ||
className: 'relative inline-block', | ||
as: 'div', | ||
}); | ||
}, | ||
}).configure({ | ||
suggestion: getVariableSuggestions(calculateVariables, VARIABLE_TRIGGER_CHARACTER, MailyVariablesList), | ||
}), |
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 you override the default Maily variable and suggestions
// IMPORTANT: do not close the sheet if the interact outside is from the maily variable list | ||
if (e.target instanceof HTMLDivElement && e.target.className.includes('tippy-box')) { | ||
return; | ||
} |
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 problem: Maily renders the variables dropdown with tippy
which renders it outside the normal tree flow and at the bottom inside the body. Clicking inside of the list is treated as clicking outside the sidebar, which results in closing the sidebar.
I wasn't able to find a better solution to this.
const [hoveredOptionIndex, setHoveredOptionIndex] = useState(0); | ||
const variablesListRef = useRef<HTMLUListElement>(null); | ||
|
||
const { variablesListRef, hoveredOptionIndex, next, prev, reset, focusFirst } = useListKeyboardNavigation({ |
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.
Do we need a a separate hook for this? Since variables-list is a reusable component I'd expect the hook to live in there.
apps/dashboard/src/components/conditions-editor/variable-select.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/components/primitives/control-input/control-input.tsx
Show resolved
Hide resolved
className={`bg-url-code bg-repeat-no-repeat h-[calc(1em-2px)] w-[calc(1em-2px)] min-w-[calc(1em-2px)] bg-[url("/images/code.svg")] bg-contain bg-center`} | ||
></span> | ||
<span className="leading-[1.2]">{variable}</span> | ||
{hasFilters && <span className="bg-feature-base h-[0.275em] w-[0.275em] rounded-full" />} |
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.
Nit-picky one, we can do the dot with an after pseudo element to remove the need of an extra span.
apps/dashboard/src/components/workflow-editor/steps/email/extensions/maily-variables-list.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/components/workflow-editor/steps/email/maily.tsx
Outdated
Show resolved
Hide resolved
|
||
import { useState } from 'react'; | ||
|
||
export const useVariableListKeyboardNavigation = ({ maxIndex }: { maxIndex: number }) => { |
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'd move the hook inside the component, the file is not that big in total, <200 lines.
What changed? Why was the change needed?
Reuse and align the variable component in the email editor.
Screenshots
Screen.Recording.2025-01-30.at.16.41.13.mov