-
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(web): reusable sms preview component #5173
Changes from 1 commit
fcd7932
d5f8f68
870a165
752f691
0ddf3fb
12453f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { useMutation, UseMutationOptions } from '@tanstack/react-query'; | ||
import { useCallback } from 'react'; | ||
import { errorMessage } from '@novu/design-system'; | ||
import { IEmailBlock } from '@novu/shared'; | ||
import { IS_DOCKER_HOSTED } from '@novu/shared-web'; | ||
|
||
import { previewSms } from '../content-templates'; | ||
|
||
type PayloadType = { | ||
content?: string | IEmailBlock[]; | ||
payload: string; | ||
locale?: string; | ||
}; | ||
|
||
type ResultType = { content: string }; | ||
|
||
type ErrorType = { error: string; message: string; statusCode: number }; | ||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export const usePreviewSms = (options: UseMutationOptions<ResultType, ErrorType, PayloadType> = {}) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hook to compile SMS template. |
||
const { mutateAsync, isLoading } = useMutation<ResultType, ErrorType, PayloadType>( | ||
({ content, payload, locale }) => previewSms({ content, payload, locale }), | ||
{ | ||
onError: (e: any) => { | ||
errorMessage(e.message || 'Unexpected error'); | ||
}, | ||
onSuccess: (result, variables, context) => { | ||
options?.onSuccess?.(result, variables, context); | ||
}, | ||
} | ||
); | ||
|
||
const getSmsPreview = useCallback( | ||
({ content, payload, locale }: PayloadType) => { | ||
if (IS_DOCKER_HOSTED) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 suggestion: Could you please add a comment in-code explaining why we do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a general pattern we use when something should not be available for the self-hosted users There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it reads logically, it might be good to extract this structure to a single helper function to ensure we apply it consistently and with documentation / comments There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's have a tech debt ticket for this as we have many places around the code ;) |
||
return; | ||
} | ||
|
||
return mutateAsync({ | ||
content, | ||
payload, | ||
locale, | ||
}); | ||
}, | ||
[mutateAsync] | ||
); | ||
|
||
return { | ||
getSmsPreview, | ||
isLoading, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,33 @@ | ||
import styled from '@emotion/styled'; | ||
import { SelectItemProps, Group } from '@mantine/core'; | ||
import { Select, Text } from '@novu/design-system'; | ||
import { Select, ISelectProps, Text } from '@novu/design-system'; | ||
import { forwardRef } from 'react'; | ||
import { IS_DOCKER_HOSTED } from '../../../../config'; | ||
|
||
const rightSectionWidth = 20; | ||
|
||
export function LocaleSelect({ locales, value, isLoading, onLocaleChange }) { | ||
export function LocaleSelect({ | ||
locales, | ||
value, | ||
isLoading, | ||
onLocaleChange, | ||
className, | ||
dropdownPosition, | ||
}: { | ||
locales: { langName: string; langIso: string }[]; | ||
value?: string; | ||
isLoading?: boolean; | ||
onLocaleChange: (val: string) => void; | ||
className?: string; | ||
dropdownPosition?: ISelectProps['dropdownPosition']; | ||
}) { | ||
Comment on lines
+9
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added missing types and a new property |
||
// Do not render locale select if self-hosted or no locale or only one locale | ||
if (IS_DOCKER_HOSTED || locales.length < 2) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<SelectContainer> | ||
<SelectContainer className={className}> | ||
<Select | ||
itemComponent={SelectItem} | ||
data={locales?.map((locale) => { | ||
|
@@ -28,11 +42,14 @@ export function LocaleSelect({ locales, value, isLoading, onLocaleChange }) { | |
searchable | ||
withinPortal | ||
onChange={(val) => { | ||
if (!val || Array.isArray(val)) return; | ||
|
||
onLocaleChange(val); | ||
}} | ||
value={value} | ||
variant="unstyled" | ||
rightSectionWidth={rightSectionWidth} | ||
dropdownPosition={dropdownPosition} | ||
/> | ||
</SelectContainer> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
import styled from '@emotion/styled'; | ||
import { Skeleton } from '@mantine/core'; | ||
import { colors, PencilOutlined } from '@novu/design-system'; | ||
import { MouseEventHandler, useState } from 'react'; | ||
|
||
const SmsBubbleHolder = styled.div` | ||
position: relative; | ||
`; | ||
|
||
const SmsBubbleContainer = styled.div<{ isBlur: boolean; isError: boolean }>` | ||
position: relative; | ||
width: fit-content; | ||
min-width: 160px; | ||
min-height: 36px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. set the min-width because on the hover we show blurred background and edit button which needs some space
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
padding: 8px 12px; | ||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
border-radius: 20px; | ||
background: ${({ isError }) => (isError ? colors.error : '#51ba52')}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 suggestion: Could we please check with Nik to make sure this will be part of our next color palette then? (I think we already have the base palette defined somewhere) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will talk to him |
||
color: ${colors.white}; | ||
font-size: 14px; | ||
line-height: 20px; | ||
filter: ${({ isBlur }) => (isBlur ? 'blur(4px)' : 'none')}; | ||
|
||
&::before { | ||
content: ''; | ||
position: absolute; | ||
bottom: -2px; | ||
right: -7px; | ||
height: 20px; | ||
border-right: 20px solid ${({ isError }) => (isError ? colors.error : '#51ba52')}; | ||
border-bottom-left-radius: 16px 14px; | ||
transform: translate(0, -2px); | ||
} | ||
|
||
&::after { | ||
content: ''; | ||
position: absolute; | ||
z-index: 1; | ||
bottom: -2px; | ||
right: -46px; | ||
width: 16px; | ||
height: 20px; | ||
background: ${({ theme }) => (theme.colorScheme === 'dark' ? '#4b4b51' : colors.white)}; | ||
border-bottom-left-radius: 10px; | ||
transform: translate(-30px, -2px); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the CSS magic to render the message bubble tail There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 suggestion: Could you please add some comments to the code to explain the purpose / use case of this magic? |
||
`; | ||
|
||
const BubbleText = styled.div` | ||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
display: -webkit-box; | ||
-webkit-line-clamp: 3; | ||
-webkit-box-orient: vertical; | ||
`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the bubble will show max 3 lines and with ellipsis at the end if the text would be longer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great! thanks! 🙌 |
||
|
||
const Delivered = styled.span` | ||
position: absolute; | ||
bottom: -24px; | ||
right: 0; | ||
font-size: 12px; | ||
font-weight: 500; | ||
color: #909093; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❔ question: Is this another potential "one-off" color? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unfortunately yes, but in this case I think we can change it to the closest we have, I will do that |
||
`; | ||
|
||
const Error = styled.span` | ||
width: max-content; | ||
position: absolute; | ||
bottom: -24px; | ||
right: -6px; | ||
font-size: 12px; | ||
font-weight: 500; | ||
color: ${colors.error}; | ||
`; | ||
|
||
const SkeletonContainer = styled.div` | ||
display: flex; | ||
gap: 10px; | ||
`; | ||
|
||
const SkeletonFirstRect = styled(Skeleton)` | ||
width: 80px; | ||
height: 20px; | ||
|
||
&::before { | ||
background: rgba(255, 255, 255, 0.24); | ||
} | ||
|
||
&::after { | ||
background: rgba(255, 255, 255, 0.3); | ||
} | ||
`; | ||
|
||
const SkeletonSecondRect = styled(SkeletonFirstRect)` | ||
width: 40px; | ||
`; | ||
|
||
const EditLabel = styled.button` | ||
position: absolute; | ||
inset: 0; | ||
z-index: 2; | ||
min-width: 120px; | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
gap: 8px; | ||
font-size: 14px; | ||
font-weight: 700; | ||
padding: 0; | ||
color: ${colors.white}; | ||
cursor: pointer; | ||
outline: none; | ||
border: none; | ||
background: transparent; | ||
`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the button that is shown over the message bubble on hover |
||
|
||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
interface ISmsBubbleProps { | ||
className?: string; | ||
text?: string; | ||
isLoading?: boolean; | ||
onEditClick?: MouseEventHandler<HTMLButtonElement>; | ||
error?: string; | ||
} | ||
|
||
export const SmsBubble: React.FC<ISmsBubbleProps> = ({ className, text, isLoading, onEditClick, error }) => { | ||
const [isHovered, setIsHovered] = useState(false); | ||
const isError = !!error; | ||
|
||
const onEditClickHandler: MouseEventHandler<HTMLButtonElement> = (e) => { | ||
e.stopPropagation(); | ||
e.preventDefault(); | ||
onEditClick?.(e); | ||
}; | ||
Comment on lines
+28
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do not propagate click events to the form |
||
|
||
return ( | ||
<SmsBubbleHolder | ||
className={className} | ||
onMouseEnter={() => setIsHovered(true)} | ||
onMouseLeave={() => setIsHovered(false)} | ||
Comment on lines
+37
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 thought: Sorry it didn't catch my eye the first time around, but this could be useful to extract to its own hook since we may have to re-use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, will do that 🙌 |
||
> | ||
{isHovered && ( | ||
<EditLabel onClick={onEditClickHandler}> | ||
<PencilOutlined /> | ||
Edit message | ||
</EditLabel> | ||
)} | ||
<SmsBubbleContainer isError={isError} isBlur={isHovered}> | ||
{isLoading ? ( | ||
<SkeletonContainer> | ||
<SkeletonFirstRect /> | ||
<SkeletonSecondRect /> | ||
</SkeletonContainer> | ||
) : ( | ||
<BubbleText title={text}>{text}</BubbleText> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❔ question: What is the expected behavior if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually, in the place where I used this component, it would never be undefined, but I've added the default value |
||
)} | ||
{isError ? <Error>{error}</Error> : <Delivered>Delivered</Delivered>} | ||
</SmsBubbleContainer> | ||
</SmsBubbleHolder> | ||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import styled from '@emotion/styled'; | ||
import { colors } from '@novu/design-system'; | ||
import { useAuthController, useDataRef } from '@novu/shared-web'; | ||
import { useEffect, useState } from 'react'; | ||
import { useFormContext, useWatch } from 'react-hook-form'; | ||
|
||
import { useGetLocalesFromContent, usePreviewSms } from '../../../api/hooks'; | ||
import { LocaleSelect } from '../../../components/workflow/Preview/common'; | ||
import { useNavigateToStepEditor } from '../hooks/useNavigateToStepEditor'; | ||
import { useStepFormErrors } from '../hooks/useStepFormErrors'; | ||
import { useStepFormPath } from '../hooks/useStepFormPath'; | ||
import { IForm } from './formTypes'; | ||
import { MobileSimulator } from './phone-simulator'; | ||
import { SmsBubble } from './SmsBubble'; | ||
|
||
const BodyContainer = styled.div` | ||
display: flex; | ||
flex-direction: column; | ||
align-items: flex-end; | ||
margin: auto 20px 40px 20px; | ||
LetItRock marked this conversation as resolved.
Show resolved
Hide resolved
|
||
`; | ||
|
||
const LocaleSelectStyled = styled(LocaleSelect)` | ||
.mantine-Select-input { | ||
color: ${({ theme }) => (theme.colorScheme === 'dark' ? colors.white : colors.B60)}; | ||
} | ||
|
||
.mantine-Input-rightSection { | ||
color: ${({ theme }) => (theme.colorScheme === 'dark' ? colors.white : colors.B60)} !important; | ||
antonjoel82 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
`; | ||
|
||
export const SmsPreview = () => { | ||
antonjoel82 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { control } = useFormContext<IForm>(); | ||
const path = useStepFormPath(); | ||
const error = useStepFormErrors(); | ||
const templateContent = useWatch({ | ||
name: `${path}.template.content`, | ||
control, | ||
}); | ||
const { organization } = useAuthController(); | ||
const [previewContent, setPreviewContent] = useState(templateContent as string); | ||
const [selectedLocale, setSelectedLocale] = useState(''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 thought: These would be helpful in code too! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll refactor this code so additional comments won't be needed. |
||
const previewData = useDataRef({ templateContent }); | ||
const templateContentError = error?.template?.content?.message; | ||
|
||
const { data: locales, isLoading: areLocalesLoading, getLocalesFromContent } = useGetLocalesFromContent(); | ||
const { isLoading: isPreviewContentLoading, getSmsPreview } = usePreviewSms({ | ||
onSuccess: (result) => { | ||
setPreviewContent(result.content); | ||
}, | ||
}); | ||
|
||
const { navigateToStepEditor } = useNavigateToStepEditor(); | ||
|
||
useEffect(() => { | ||
getLocalesFromContent({ | ||
content: previewData.current.templateContent, | ||
}); | ||
}, [getLocalesFromContent, previewData]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fetch locales from the template, will be called only once during the initial mount |
||
|
||
useEffect(() => { | ||
getSmsPreview({ | ||
content: previewData.current.templateContent, | ||
payload: '', | ||
locale: selectedLocale, | ||
}); | ||
}, [selectedLocale, previewData, getSmsPreview]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. compile the template for the preview |
||
|
||
const onLocaleChange = (locale) => { | ||
setSelectedLocale(locale); | ||
}; | ||
|
||
const isBubbleLoading = !templateContentError && isPreviewContentLoading; | ||
|
||
return ( | ||
<MobileSimulator> | ||
<BodyContainer> | ||
<LocaleSelectStyled | ||
isLoading={areLocalesLoading} | ||
locales={locales} | ||
value={selectedLocale || organization?.defaultLocale} | ||
onLocaleChange={onLocaleChange} | ||
dropdownPosition="top" | ||
/> | ||
<SmsBubble | ||
onEditClick={navigateToStepEditor} | ||
isLoading={isBubbleLoading} | ||
text={previewContent} | ||
error={templateContentError} | ||
/> | ||
</BodyContainer> | ||
</MobileSimulator> | ||
); | ||
}; |
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.
❔ question: Do we have a more restricted type for
Locale
that could be used here?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.
unfortunately, we don't :/ might be hard to introduce now without refactoring
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.
Sounds good, created a ticket for the future: https://linear.app/novu/issue/NV-3499/create-a-reusable-typescript-type-for-locale