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

feat: make sender name an input field for email card #2769

Merged
merged 4 commits into from Feb 16, 2023

Conversation

davidsoderberg
Copy link
Contributor

What change does this PR introduce?

So sender name can be changed from each email template if needed empty field uses default from integration.

@linear
Copy link

linear bot commented Feb 15, 2023

NV-1473 Option of from and sender name in template level

As of now, we ask from and Sender Name while user connect email provider in integration store.

These values are applied to all templates and all subscribers. There should be an option of overriding these values at template level.

Because Each template can be of different type, having different type of email content and for different purpose.

Base automatically changed from nv-1527-feature-support-of-multiple-email-ids-in to next February 15, 2023 10:47
Copy link
Contributor

@p-fernandez p-fernandez left a comment

Choose a reason for hiding this comment

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

🌟

2 considerations, but looks good!

...integration,
credentials: {
...integration.credentials,
senderName: senderName && senderName.length > 0 ? senderName : integration.credentials.senderName,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
senderName: senderName && senderName.length > 0 ? senderName : integration.credentials.senderName,
senderName: senderName?.length > 0 ? senderName : integration.credentials.senderName,

Just saving characteres.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this typescript compliance about senderName possible being undefined...

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right. I missed that for not trying the PR in my local.

@scopsy
Copy link
Contributor

scopsy commented Feb 16, 2023

@davidsoderberg could you please send a loom/photo of how this new input looks like and behaves?

@davidsoderberg
Copy link
Contributor Author

Screenshot 2023-02-16 at 10 11 25

Screenshot 2023-02-16 at 10 11 33

Sure I am not sure the user will understand what the field is used for...

@scopsy
Copy link
Contributor

scopsy commented Feb 16, 2023

@davidsoderberg aggree, what if we will add the input title similar to the "Email Layout" one? And remove the Inbox view header and will keep it only the preview tab ?

@davidsoderberg
Copy link
Contributor Author

@davidsoderberg aggree, what if we will add the input title similar to the "Email Layout" one? And remove the Inbox view header and will keep it only the preview tab ?

Right now we do not have in preview so lets think about that part when we get to it... I was thinking of tooltip instead to save space, what do you think about that?

@davidsoderberg
Copy link
Contributor Author

davidsoderberg commented Feb 16, 2023

Screenshot 2023-02-16 at 12 22 13

Maybe we can remove the date now it do not look like an inbox row anymore?

@scopsy
Copy link
Contributor

scopsy commented Feb 16, 2023

I think it makes much more sense @davidsoderberg, we can have the "date" in the preview section.

@scopsy
Copy link
Contributor

scopsy commented Feb 16, 2023

@davidsoderberg could you add a tooltip near the Sender Name to say that if empty we will use the one from integration page?

@davidsoderberg
Copy link
Contributor Author

Screenshot 2023-02-16 at 12 38 42

@davidsoderberg davidsoderberg added this pull request to the merge queue Feb 16, 2023
Merged via the queue into next with commit ed2ca05 Feb 16, 2023
@davidsoderberg davidsoderberg deleted the nv-1473-option-of-from-and-sender-name-in branch February 16, 2023 11:41
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.

None yet

3 participants