Skip to content

Conversation

@LetItRock
Copy link
Contributor

What changed? Why was the change needed?

THIS PR REQUIRES CHANGES IN MAILY: novuhq/maily.to#23

In this PR a few issues were fixed:

  • the pill variable text is not visible when used with the Mailly black button
  • the digest variables should not be accessible inside the repeat block
  • in the bubble the current variable was not accessible
  • the array item access in the button that is inside the repeat block was not working
  • the aliasFor was not set on the buttons as we treat them differently so it resulted that the current variable was not working

Screenshots

Screen.Recording.2025-04-28.at.18.48.24.mov

@linear
Copy link

linear bot commented Apr 28, 2025

@netlify
Copy link

netlify bot commented Apr 28, 2025

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

Name Link
🔨 Latest commit 2fa08d8
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/681209c86b3ec6000803abab
😎 Deploy Preview https://deploy-preview-8225.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.

<VariableIcon variableName={variableName} hasError={!!issues} />
<span className="leading-1 max-w-[24ch] truncate" title={displayVariableName}>
{/* INFO: Keep the color defined on the span to avoid overriding it in maily components for example button */}
<span className="leading-1 text-text-sub max-w-[24ch] truncate" title={displayVariableName}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the pill variable text was not visible when used with the Mailly black button

Comment on lines +138 to +143
if (!isInRepeatBlock && isEnhancedDigestEnabled && addDigestVariables) {
const mappedDigestVariables = DIGEST_VARIABLES.map((variable) => ({
name: variable.name,
}));
baseVariables.push(...mappedDigestVariables);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the digest variables should not be accessible inside the repeat block

// Case 2: Bubble menu (showIf) - allow only primitives and namespaces
case VariableFrom.Bubble:
return [...primitives, ...namespaces];
return getVariables();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the bubble the current variable was not accessible

return processedNode;
}

if (isButtonNode(processedNode)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the array item access in the button that is inside the repeat block was not working

Comment on lines 277 to 283
const aliasFor = resolveRepeatBlockAlias(
isTextVariable ? (text ?? '') : (url ?? ''),
editor,
isEnhancedDigestEnabled
);
return commands.updateButton?.({ ...attrs, aliasFor: aliasFor ?? null });
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

calculate the aliasFor for the text or url buttons that are variables

Copy link
Contributor

@scopsy scopsy left a comment

Choose a reason for hiding this comment

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

👏

…nd the node marks; override maily blocks extensions to support aliasFor attribute
return processedNode;
}

if (isImageNode(processedNode)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and the same thing for the other types of nodes

Comment on lines +319 to +325
if (processedNode.marks?.length) {
processedNode.marks = this.processForEachNodes(
processedNode.marks,
iterablePath,
index
) as Array<MailyJSONMarks>;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've been missing this piece of code that processes the node marks, for example, text that is the link; so this code ensures that if we use variables in the links, it will work

Comment on lines +130 to +139
if (type === MailyContentTypeEnum.INLINE_IMAGE) {
return [
{ attr: MailyAttrsEnum.SRC, flag: MailyAttrsEnum.IS_SRC_VARIABLE },
{
attr: MailyAttrsEnum.EXTERNAL_LINK,
flag: MailyAttrsEnum.IS_EXTERNAL_LINK_VARIABLE,
},
...commonConfig,
];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems that the variables never worked on the inline_image :/


if (isEnhancedDigestEnabled) {
extensions.push(
ButtonExtension.extend({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the below piece of code allows us to hook into the node attributes update and set the aliasFor attribute that is used to resolve the variable

@LetItRock LetItRock merged commit 686f6d1 into next Apr 30, 2025
26 checks passed
@LetItRock LetItRock deleted the nv-5766-email-editor-allow-using-current-variable-in-button branch April 30, 2025 12:33
bricehemery pushed a commit to jack-agency/novu that referenced this pull request Jul 7, 2025
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.

5 participants