Skip to content
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

fix(dashboard): wait for save when switching preview tab #7299

Conversation

ChmaraX
Copy link
Contributor

@ChmaraX ChmaraX commented Dec 13, 2024

What changed? Why was the change needed?

The preview components already had the logic to wait for preview data - the issue was that it was incorrectly mounted in in-app due to different tabs implementation compared to email.

  • extracted the common tabs logic to wrapper, since it was the reason for the bug in the first place - we had slightly different implementation in each of them
  • unified email and in-app editors
tab-switch.mov

slow network:

3g-network.mov

Copy link

linear bot commented Dec 13, 2024

Comment on lines +17 to +29
const isNovuCloud = workflow.origin === WorkflowOriginEnum.NOVU_CLOUD && uiSchema;
const isExternal = workflow.origin === WorkflowOriginEnum.EXTERNAL;

const editorContent = (
<>
{isNovuCloud && <EmailEditor uiSchema={uiSchema} />}
{isExternal && (
<EmailTabsSection>
<CustomStepControls dataSchema={dataSchema} origin={workflow.origin} />
</EmailTabsSection>
)}
</>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more explicit conditions so its unified across steps

@@ -12,8 +12,8 @@ const redirectKey = 'redirect';
const primaryActionKey = 'primaryAction';
const secondaryActionKey = 'secondaryAction';

export const InAppEditor = ({ uiSchema }: { uiSchema?: UiSchema }) => {
if (!uiSchema || uiSchema?.group !== UiSchemaGroupEnum.IN_APP) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if its better to check for uiSchema in child or parent - moved to parent for consistency with email-editor

Copy link

netlify bot commented Dec 13, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 4b08f9f
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/67604937b89cf80009b7ece7
😎 Deploy Preview https://deploy-preview-7299.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 13, 2024

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 4b08f9f
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/67604937fb977f000812373b
😎 Deploy Preview https://deploy-preview-7299.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

<span>Preview</span>
</TabsTrigger>
</TabsList>
const isNovuCloud = workflow.origin === WorkflowOriginEnum.NOVU_CLOUD && uiSchema;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our goal should be to align Dashboard and Code First workflow management. That is, uiSchema should be available in both. Let's remove it from this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently its not available in both and the uiSchema type is optional on the response, there needs to be a check somewhere and this is the only place where the check makes sense.

If we want to achieve what you are describing there needs to be additional changes on API which I don't want to include as part of this UI bugfix.

export const InAppEditor = ({ uiSchema }: { uiSchema?: UiSchema }) => {
if (!uiSchema || uiSchema?.group !== UiSchemaGroupEnum.IN_APP) {
export const InAppEditor = ({ uiSchema }: { uiSchema: UiSchema }) => {
if (uiSchema.group !== UiSchemaGroupEnum.IN_APP) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@ChmaraX ChmaraX merged commit 92b532e into next Dec 16, 2024
24 checks passed
@ChmaraX ChmaraX deleted the nv-5008-trigger-autosave-immediately-when-switching-from-editor-to branch December 16, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants