feat(web): reusable sms preview component #5173

LetItRock · 2024-02-08T23:16:36Z

What change does this PR introduce?

Reusable SMS preview component based on the new designs from the Translation Management project.

Why was this change needed?

Other information (Screenshots)

Screen.Recording.2024-02-08.at.23.53.01.mov

linear · 2024-02-08T23:16:39Z

NV-2998 🌐 Reusable SMS preview component

LetItRock · 2024-02-08T23:19:23Z

apps/web/src/api/hooks/usePreviewSms.ts

+
+type ErrorType = { error: string; message: string; statusCode: number };
+
+export const usePreviewSms = (options: UseMutationOptions<ResultType, ErrorType, PayloadType> = {}) => {


The hook to compile SMS template.

LetItRock · 2024-02-08T23:19:46Z

apps/web/src/components/workflow/Preview/common/LocaleSelect.tsx

+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'];
+}) {


added missing types and a new property dropdownPosition

LetItRock · 2024-02-08T23:20:36Z

apps/web/src/pages/templates/components/SmsBubble.tsx

+  &::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);
+  }


the CSS magic to render the message bubble tail

🎯 suggestion: Could you please add some comments to the code to explain the purpose / use case of this magic?‏

LetItRock · 2024-02-08T23:21:29Z

apps/web/src/pages/templates/components/SmsBubble.tsx

+  min-width: 160px;
+  min-height: 36px;


set the min-width because on the hover we show blurred background and edit button which needs some space

LetItRock · 2024-02-08T23:22:12Z

apps/web/src/pages/templates/components/SmsBubble.tsx

+  min-height: 36px;
+  padding: 8px 12px;
+  border-radius: 20px;
+  background: ${({ isError }) => (isError ? colors.error : '#51ba52')};


#51ba52 this is the light green color unfortunately not from the color theme we have

🎯 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)‏

will talk to him

LetItRock · 2024-02-08T23:27:54Z

apps/web/src/pages/templates/components/SmsPreview.tsx

+  const [previewContent, setPreviewContent] = useState(templateContent as string);
+  const [selectedLocale, setSelectedLocale] = useState('');


previewContent is a compiled template content shown as the text
selectedLocale is the locale to which the template would be compiled

💭 thought: These would be helpful in code too!‏

I'll refactor this code so additional comments won't be needed.

LetItRock · 2024-02-08T23:29:09Z

apps/web/src/pages/templates/components/SmsPreview.tsx

+  useEffect(() => {
+    getLocalesFromContent({
+      content: previewData.current.templateContent,
+    });
+  }, [getLocalesFromContent, previewData]);


fetch locales from the template, will be called only once during the initial mount

LetItRock · 2024-02-08T23:29:25Z

apps/web/src/pages/templates/components/SmsPreview.tsx

+  useEffect(() => {
+    getSmsPreview({
+      content: previewData.current.templateContent,
+      payload: '',
+      locale: selectedLocale,
+    });
+  }, [selectedLocale, previewData, getSmsPreview]);


compile the template for the preview

LetItRock · 2024-02-08T23:34:25Z

apps/web/src/pages/templates/components/phone-simulator/MobileSimulator.tsx

+export const MobileSimulator: React.FC = ({ children }) => {
+  const [isIOS, setIsIOS] = useState(true);
+  const { colorScheme } = useMantineColorScheme();
+
+  return (
+    <MobileSimulatorBody>
+      {isIOS ? <Notch /> : <Camera />}
+      <IndicatorsContainer>
+        <TimeIconStyled isVisible={isIOS} />
+        {isIOS ? <IOSIndicatorsIcon /> : <AndroidIndicatorsIcon />}
+      </IndicatorsContainer>
+      <SwitchContainer>
+        <PhonePlatformSwitch checked={isIOS} onChange={() => setIsIOS((old) => !old)} />
+      </SwitchContainer>
+      {children}
+      {isIOS ? (
+        <IOSKeyboard isDarkMode={colorScheme === 'dark'} />
+      ) : (
+        <AndroidKeyboard isDarkMode={colorScheme === 'dark'} />
+      )}
+    </MobileSimulatorBody>
+  );
+};


tried a few approaches here, exported phone svg and webp images, but the problem that I faced is with changing it for our needs, like for ex. in the Push preview we would need to have a background... also there are different themes and platforms, so in total would be 4 images just for sms, and 4 for push then...
that's why I decided to code it with css and only have some parts included with svg... it's less scalable, but for now we don't need this...

LetItRock · 2024-02-08T23:35:43Z

apps/web/src/pages/templates/components/phone-simulator/PhonePlatformSwitch.tsx

+      <SwitchInput id="phonePlatformSwitch" ref={ref} type="checkbox" checked={checked} onChange={onChange} />
+      <ButtonsContainer htmlFor="phonePlatformSwitch">


old good css trick to use input + label

antonjoel82

Code looks pretty good overall! Will do behavior validation tomorrow morning in another pass.

Some high-level thoughts:

We should aim for rem where possible IMO
The assets related to the Mobile views looked not insignificantly large... when we begin chunking the application, I think we will want to make sure those are not in the main bundle

antonjoel82 · 2024-02-09T00:08:10Z

apps/web/src/api/content-templates.ts

+}: {
+  content?: string | IEmailBlock[];
+  payload: string;
+  locale?: string;


❔ question: Do we have a more restricted type for Locale that could be used here?‏

unfortunately, we don't :/ might be hard to introduce now without refactoring

Sounds good, created a ticket for the future: https://linear.app/novu/issue/NV-3499/create-a-reusable-typescript-type-for-locale

antonjoel82 · 2024-02-09T00:09:54Z

apps/web/src/api/hooks/usePreviewSms.ts

+
+  const getSmsPreview = useCallback(
+    ({ content, payload, locale }: PayloadType) => {
+      if (IS_DOCKER_HOSTED) {


🎯 suggestion: Could you please add a comment in-code explaining why we do this?‏

this is a general pattern we use when something should not be available for the self-hosted users

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

let's have a tech debt ticket for this as we have many places around the code ;)

apps/web/src/api/hooks/usePreviewSms.ts

apps/web/src/pages/templates/components/ChannelPreview.tsx

apps/web/src/pages/templates/components/SmsBubble.tsx

apps/web/src/pages/templates/components/phone-simulator/PhonePlatformSwitch.tsx

antonjoel82 · 2024-02-09T00:43:27Z

apps/web/src/utils/notifications.tsx

-import { errorMessage, successMessage } from '@novu/design-system';
+import { errorMessage as errorMessageRoot, successMessage as successMessageRoot } from '@novu/design-system';

-export { errorMessage, successMessage };
+export const errorMessage = errorMessageRoot;
+export const successMessage = successMessageRoot;


❔ question: Why make this abstraction here?‏

I had an error in the console that somehow it wasn't able to find these methods when doing imports, it was weird and only this helped

antonjoel82 · 2024-02-09T00:44:10Z

apps/web/src/pages/templates/hooks/useNavigateToStepEditor.ts

+    const isVariant = !!variantUuid;
+    let navigatePath = `${basePath}/${channel}/${stepUuid}`;
+    if (isVariant) {
+      navigatePath += `/variants/${variantUuid}`;


🤓 nitpick: Is there a constant we can derive this from?‏

unfortunately, we don't, because it's a dynamic path

Sorry, by that I meant something more like /${VARIANTS_PATH}/${variantUuid}

…reusable-sms-preview

…preview

antonjoel82 · 2024-02-09T17:50:22Z

Looks and works great overall! Nice job.

A couple UX things:

I think on phones, the message bubbles do have text left-aligned, but generally are sized to fit content, so we may want to shrink the minimum width
It may be worth the discussion with Nik first, but incoming messages (both Android and iOS) are always on the left and gray instead, so it might fit better with the real world if it looks like a received message
Lastly, in mock-ups it looks like the "Edit message" + blur are centered in the entire screen rather than within the message bubble; did we decide to deviate from that?

LetItRock · 2024-02-12T22:46:26Z

I think on phones, the message bubbles do have text left-aligned, but generally are sized to fit content, so we may want to shrink the minimum width

It may be worth the discussion with Nik first, but incoming messages (both Android and iOS) are always on the left and gray instead, so it might fit better with the real world if it looks like a received message

Lastly, in mock-ups it looks like the "Edit message" + blur are centered in the entire screen rather than within the message bubble; did we decide to deviate from that?

These are the things I discussed with Nikolay and we came up with this implementation. Mainly because of the issues we had with the overlay showing on the entire phone (it doesn't allow change language, frustration on hover, etc.).

antonjoel82

Thanks for the changes and responses!

antonjoel82 · 2024-02-12T23:40:16Z

apps/web/src/api/hooks/useGetLocalesFromContent.ts

-    }
-  );
+  } = useMutation<ILocale[], IResponseError, Payload>(({ content }) => getLocalesFromContent({ content }), {
+    onError: (e: any) => {


🤓 nitpick: unknown is technically recommended by TypeScript here (introduced first in 4.4) ‏

Suggested change

onError: (e: any) => {

onError: (e: unknown) => {

it should be defined tho IResponseError, I will remove that :)

antonjoel82 · 2024-02-12T23:46:14Z

apps/web/src/api/hooks/usePreviewSms.ts

+
+  const getSmsPreview = useCallback(
+    ({ content, payload, locale }: PayloadType) => {
+      if (IS_DOCKER_HOSTED) {


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

apps/web/src/pages/templates/components/SmsBubble.styles.tsx

antonjoel82 · 2024-02-12T23:50:00Z

apps/web/src/pages/templates/components/SmsBubble.tsx

+}
+
+export const SmsBubble: React.FC<ISmsBubbleProps> = ({ className, text = '', isLoading, onEditClick, error }) => {
+  const [isHovered, setIsHovered] = useState(false);


Suggested change

}

export const SmsBubble: React.FC<ISmsBubbleProps> = ({ className, text = '', isLoading, onEditClick, error }) => {

const [isHovered, setIsHovered] = useState(false);

}

export const SmsBubble: React.FC<ISmsBubbleProps> = ({ className, text = '', isLoading, onEditClick, error }) => {

const [isHovered, setIsHovered] = useState<boolean>(false);

In my previous position, we ended up with typing issues everywhere we didn't have explicit type params, so my preference is to always use them to be safe. Especially because in theory this could be false | null or false | undefined instead of boolean

The type is derived here from the value and it won't allow setting the state for undefined or null. But I'm not sure whether you mean something else.

antonjoel82 · 2024-02-12T23:51:14Z

apps/web/src/pages/templates/components/SmsBubble.tsx

+      onMouseEnter={() => setIsHovered(true)}
+      onMouseLeave={() => setIsHovered(false)}


💭 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 useHover behavior elsewhere‏

sure, will do that 🙌

antonjoel82 · 2024-02-12T23:54:29Z

apps/web/src/pages/templates/components/phone-simulator/MobileSimulator.styles.tsx

+`;
+
+export const TimeIconStyled = styled(({ isVisible }: { isVisible: boolean }) => (
+  <TimeIcon style={{ opacity: isVisible ? 1 : 0 }} />


❔ question: Do we specifically want this to still "occupy" its space even when not visible?‏

yes, otherwise we would have to position the indicators component differently in the flex container

feat(web): reusable sms preview component

fcd7932

LetItRock self-assigned this Feb 8, 2024

github-actions bot added @novu/web @novu/design-system labels Feb 8, 2024

LetItRock commented Feb 8, 2024

View reviewed changes

antonjoel82 reviewed Feb 9, 2024

View reviewed changes

Merge branch 'nv-2995-reusable-email-preview-component' into nv-2998-…

d5f8f68

…reusable-sms-preview

Base automatically changed from nv-2995-reusable-email-preview-component to translation-management-beta February 9, 2024 13:06

Merge branch 'translation-management-beta' into nv-2998-reusable-sms-…

870a165

…preview

feat(web): reusable sms preview suggestions from the pr #5173

752f691

github-actions bot added the @novu/shared label Feb 12, 2024

LetItRock requested review from BiswaViraj and antonjoel82 February 12, 2024 22:38

chore(web): renamed the hook

0ddf3fb

antonjoel82 approved these changes Feb 13, 2024

View reviewed changes

chore(web): reusable sms preview suggestions from the pr #5173

12453f9

LetItRock merged commit 24526c2 into translation-management-beta Feb 13, 2024
23 checks passed

LetItRock deleted the nv-2998-reusable-sms-preview branch February 13, 2024 12:48


		type ErrorType = { error: string; message: string; statusCode: number };

		export const usePreviewSms = (options: UseMutationOptions<ResultType, ErrorType, PayloadType> = {}) => {

		const [previewContent, setPreviewContent] = useState(templateContent as string);
		const [selectedLocale, setSelectedLocale] = useState('');

		<SwitchInput id="phonePlatformSwitch" ref={ref} type="checkbox" checked={checked} onChange={onChange} />
		<ButtonsContainer htmlFor="phonePlatformSwitch">

		onMouseEnter={() => setIsHovered(true)}
		onMouseLeave={() => setIsHovered(false)}

feat(web): reusable sms preview component #5173

feat(web): reusable sms preview component #5173

Conversation

LetItRock commented Feb 8, 2024

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Feb 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonjoel82 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonjoel82 commented Feb 9, 2024 • edited Loading

LetItRock commented Feb 12, 2024 • edited Loading

antonjoel82 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonjoel82 commented Feb 9, 2024 •

edited

Loading

LetItRock commented Feb 12, 2024 •

edited

Loading