fix(web): fixed the template editor react-hook-form dirty state #2674

LetItRock · 2023-02-03T02:53:38Z

What change does this PR introduce?

Notification Template Editor - fixed the react-hook-form dirty state issue, which was causing the create/update button to not work properly.

The changes:

refactored code and moved the logic to context
applied the default values for the fields
changed the form controllers to not point to values that are objects, arrays
fixed issues with variable management

Why was this change needed?

Other information (Screenshots)

Screen.Recording.2023-02-03.at.03.51.33.mov

linear · 2023-02-03T02:53:40Z

NV-1587 Changes made prompt shouldn't be shown

To reproduce:

Visit a saved template
Go to existing step edit screen
Go back to the workflow page
Click on Test Workflow
A prompt is shown even when I haven't done any change.

ghost · 2023-02-03T02:54:38Z

👇 Click on the image for a new way to code review

Make big changes easier — review code in small groups of related files
Know where to start — see the whole change at a glance
Take a code tour — explore the change with an interactive tour
Make comments and review — all fully sync’ed with github

Try it now!

Legend

LetItRock · 2023-02-03T09:07:11Z

apps/web/cypress/tests/changes.spec.ts

@@ -103,7 +103,7 @@ function createNotification() {

  cy.getByTestId('emailSubject').type('this is email subject');

-  cy.getByTestId('submit-btn').click();
+  cy.getByTestId('notification-template-submit-btn').click();


template editor: renamed the data-test-id name for the Create/Update button

LetItRock · 2023-02-03T09:09:33Z

apps/web/package.json

@@ -37,6 +37,7 @@
    "@emotion/react": "^11.7.1",
    "@emotion/styled": "^11.6.0",
    "@handlebars/parser": "^2.1.0",
+    "@hookform/devtools": "^4.3.0",


added this library to debug the react-hook-form

LetItRock · 2023-02-03T09:10:26Z

apps/web/package.json

@@ -77,6 +78,7 @@
    "lodash.clonedeep": "^4.5.0",
    "lodash.get": "^4.3.2",
    "lodash.set": "^4.3.2",
+    "lodash.isequal": "^4.5.0",


added this library to compare the variables arrays when parsing template fields, more about that later

LetItRock · 2023-02-03T09:12:59Z

apps/web/src/components/templates/TemplateEditor.tsx

 import { ActivePageEnum } from '../../pages/templates/editor/TemplateEditorPage';
 import { TemplatePushEditor } from './TemplatePushEditor';
 import { TemplateChatEditor } from './chat-editor/TemplateChatEditor';

 export const TemplateEditor = ({ activePage, templateId, activeStep }) => {
  const { integrations } = useActiveIntegrations();
-  const { control, errors, watch } = useTemplateController(templateId);


Refactored this hook, there is no sense to hold the react-hook-form functionality inside, instead better to use useFormContext directly as we do in many other places.

LetItRock · 2023-02-03T09:14:07Z

apps/web/src/components/templates/TemplateEditorProvider.tsx

-const reducer = (state, action) => {
-  switch (action.type) {
-    case actionEditMode:
-      return {
-        ...state,
-        editMode: action.payload,
-      };
-    case actionIsDirty:
-      return {
-        ...state,
-        isDirty: action.payload,
-      };
-    case actionIsEmbedModalVisible:
-      return {
-        ...state,
-        isEmbedModalVisible: action.payload,
-      };
-    case actionTrigger:
-      return {
-        ...state,
-        trigger: action.payload,
-      };
-    case actionCreatedTemplateId:
-      return {
-        ...state,
-        createdTemplateId: action.payload,
-      };
-    default:
-      throw new Error('Unspecified reducer action');
-  }
-};


removed the reducer from here, some of the actions were not used, so I've replaced it with the useState

LetItRock · 2023-02-03T10:06:20Z

apps/web/src/pages/templates/workflow/ReplyCallback.tsx


 export const ReplyCallback = ({ control, index, errors }) => {
  const { environment } = useEnvController();
-  const { getValues } = useFormContext();
-  const replyCallbackActive = getValues(`steps.${index}.replyCallback.active`);


getValues only have an acces to the value during the render, but it doesn't listen to the value changes

LetItRock · 2023-02-03T10:06:52Z

apps/web/src/pages/templates/workflow/SideBar/SelectedStep.tsx

@@ -90,7 +89,7 @@ export function SelectedStep({
              <Divider my={30} />
              {steps.map((i, index) => {
                return (
-                  <When truthy={index === activeStep}>
+                  <When key={index} truthy={index === activeStep}>


fixed the react warning in the console

LetItRock · 2023-02-03T10:07:16Z

apps/web/src/pages/templates/workflow/SideBar/SelectedStep.tsx

+const FilterButton = styled(Button)`
+  background: ${({ theme }) => (theme.colorScheme === 'dark' ? colors.B20 : colors.white)};
  box-shadow: 0px 5px 20px rgb(0 0 0 / 20%);
  :hover {
-    background-color: ${({ dark }) => (dark ? colors.B20 : colors.white)};
+    background-color: ${({ theme }) => (theme.colorScheme === 'dark' ? colors.B20 : colors.white)};


fixed another react warning in the console

LetItRock · 2023-02-03T10:07:44Z

apps/web/src/pages/templates/workflow/WorkflowEditorPage.tsx

              return (
-                <When truthy={index === activeStep}>
+                <When key={step._id || step.id} truthy={index === activeStep}>


another react warning

LetItRock · 2023-02-03T10:09:39Z

libs/testing/src/notification-template.service.ts

+        variables: [
+          {
+            defaultValue: '',
+            name: 'firstName',
+            required: false,
+            type: 'String',
+          },
+        ],


Added the missing variables array, after I've fixed all the variable management issues the tests were not passing because of this causing the form to be dirty.

p-fernandez

🌟
Great great job. Very good decisions. 👏🏻

apps/web/cypress/tests/notification-editor/main-functionality.spec.ts

apps/web/src/components/templates/TemplateEditorProvider.tsx

p-fernandez · 2023-02-03T11:19:16Z

apps/web/src/components/templates/TemplateEditorProvider.tsx

+            },
+          });
+          setTrigger(response.triggers[0]);
+          reset(data);


What's the reason that here we reset the data but in line 217 we reset the payloadToCreate? I would expect data to be reset in both. What am I missing?

not sure, it was like that before... I feel it doesn't have an impact on the form at all because after creation we redirect the user to the /templates route

apps/web/src/components/templates/TemplateEditorProvider.tsx

apps/web/src/components/templates/email-editor/ButtonRowContent.tsx

p-fernandez · 2023-02-03T11:27:47Z

apps/web/src/components/templates/email-editor/EmailContentCard.tsx

@@ -27,7 +26,7 @@ export function EmailContentCard({
  const { readonly } = useEnvController();
  const { control, setValue, watch } = useFormContext(); // retrieve all hook methods
  const contentType = watch(`steps.${index}.template.contentType`);
-  const [activeTab, setActiveTab] = useState<string | null>(EDITOR);
+  const activeTab = contentType === 'customHtml' ? CUSTOM_CODE : EDITOR;


Time ago I created a type MessageTemplateContentType that holds customHtml and editor. I think I should convert it to an enum and be used in situations like here. What would it be your take on that?

yeah, I saw that one, but obviously couldn't use it here, so I went the easiest way 🥲 but saw couple more places where it might be useful

apps/web/src/components/templates/email-editor/EmailMessageEditor.tsx

apps/web/src/components/templates/email-editor/TextRowContent.tsx

apps/web/src/components/templates/notification-setting-form/NotificationSettingsForm.tsx

ainouzgali · 2023-02-07T17:13:14Z

@LetItRock This is such amazing work! This will improve significantly user experience and sooo many bugs. Yay!
Can this be merged? Or are you still working on it?

ainouzgali

🚀

fix(web): fixed the template editor react-hook-form dirty state

e7bf601

LetItRock self-assigned this Feb 3, 2023

github-actions bot added @novu/shared @novu/web labels Feb 3, 2023

Merge branch 'next' into nv-1587-template-editor-react-hook-form-fix

da53036

LetItRock commented Feb 3, 2023

View reviewed changes

LetItRock added 3 commits February 3, 2023 11:10

chore(web): small adjustments

db75d1e

chore(web): the pnpm lock file

1bf7e81

chore(web): fixed failing variables management component test

38aa5c8

p-fernandez approved these changes Feb 3, 2023

View reviewed changes

ainouzgali mentioned this pull request Feb 6, 2023

[NV-1636] ✨ Polishing: New template variable properties are reset when saving workflow #2684

Closed

2 tasks

LetItRock added 2 commits February 6, 2023 23:16

Merge branch 'next' into nv-1587-template-editor-react-hook-form-fix

1628b13

chore(web): added suggestions from the pr #2674

d8fcb0b

ainouzgali approved these changes Feb 7, 2023

View reviewed changes

LetItRock merged commit d08f3be into next Feb 7, 2023

LetItRock deleted the nv-1587-template-editor-react-hook-form-fix branch February 7, 2023 17:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(web): fixed the template editor react-hook-form dirty state #2674

fix(web): fixed the template editor react-hook-form dirty state #2674

LetItRock commented Feb 3, 2023 •

edited

linear bot commented Feb 3, 2023

ghost commented Feb 3, 2023 •

edited by ghost

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

LetItRock Feb 3, 2023

p-fernandez left a comment

p-fernandez Feb 3, 2023

LetItRock Feb 6, 2023

p-fernandez Feb 3, 2023

LetItRock Feb 6, 2023

ainouzgali commented Feb 7, 2023

ainouzgali left a comment

fix(web): fixed the template editor react-hook-form dirty state #2674

fix(web): fixed the template editor react-hook-form dirty state #2674

Conversation

LetItRock commented Feb 3, 2023 • edited

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Feb 3, 2023

ghost commented Feb 3, 2023 • edited by ghost

Legend

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

p-fernandez 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

ainouzgali commented Feb 7, 2023

ainouzgali left a comment

Choose a reason for hiding this comment

LetItRock commented Feb 3, 2023 •

edited

ghost commented Feb 3, 2023 •

edited by ghost