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.

Sign up for GitHub

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

Jump to bottom

feat(rich-text): support image as links #1158

Merged
merged 5 commits into from
May 11, 2021
Merged

feat(rich-text): support image as links #1158

merged 5 commits into from
May 11, 2021

Conversation

lamkeewei
Copy link
Contributor

@lamkeewei lamkeewei commented Apr 28, 2021 •
edited
Loading

Problem

Users have been requesting to use images as links in their emails.

Solution

Features:

  • Add optional field to provide link when inserting image
  • Allow conversion to and from Draft.js state and HTML

Bug Fixes:

  • Fix missing styles for popover when clicking on image

Before & After Screenshots

Tests

Insert image with link

  • Create email campaign and insert an image with link
  • Popover when clicking on image should show link to URL
  • Saving template and revisiting template again should show that link is preserved
  • Image in message preview should also be linked

Insert image without link

  • Create email campaign and insert an image without link
  • Popover should not show any link

Insert image with variable link

  • Create protected email campaign and insert an image with variable (e.g. {{protectedlink}} as link)
  • In sent test message, the URL should be correctly substituted

@lamkeewei lamkeewei marked this pull request as draft April 28, 2021 08:35
@lamkeewei lamkeewei marked this pull request as ready for review April 29, 2021 03:53
@lamkeewei lamkeewei requested a review from miazima May 4, 2021 02:11
@miazima
Copy link
Contributor

miazima commented May 5, 2021

Tested on staging and the email sent out contains linked image and successfully navigates to correct link
https://rich-text-image.postman.gov.sg

Had issues with

  • image does not contain link in preview
  • when going back to create message step, the image popover does not contain the link and email sent out does not have link for image

@lamkeewei
Copy link
Contributor Author

Tested on staging and the email sent out contains linked image and successfully navigates to correct link
https://rich-text-image.postman.gov.sg

Had issues with

  • image does not contain link in preview
  • when going back to create message step, the image popover does not contain the link and email sent out does not have link for image

@miazima could you help provide the steps to reproduce the errors you were facing? Wasn't able to reproduce this locally. Backend and workers also need to be deployed to this branch, otherwise the data-link attribute will be stripped.

@miazima
Copy link
Contributor

miazima commented May 10, 2021

Hmm I tested on staging again, only issue I had is that the image was not linked in the preview block

@lamkeewei
Merge branch 'develop' into feat-image-links
6876fa2 
* develop:
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
  fix(backend): docker build and tsc output directory structure (#1177)
  feat: refactor msg template components; add telegram character limit (#1148)
  refactor: use shared function to initialize models (#1172)
  chore: setup scaffolding for backend tests (#940)
  1.23.0
  fix(frontend): fix frontend test flakiness (#1162)
  fix: update successful delivery status only if error does not exist (#1150)
  chore: upgrade dependencies (#1153)
@lamkeewei
Copy link
Contributor Author

Hmm I tested on staging again, only issue I had is that the image was not linked in the preview block

@miazima this should be fixed on staging! Realised the problem is that the previous Amplify build failed because of the tests timeout. This was fixed in #1162.

image

miazima
miazima approved these changes May 11, 2021
View reviewed changes
Copy link
Contributor

@miazima miazima left a comment

Choose a reason for hiding this comment

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

Tested on staging, lgtm!

@miazima miazima merged commit 4a48f22 into develop May 11, 2021
@miazima miazima deleted the feat-image-links branch May 11, 2021 03:10
lamkeewei added a commit that referenced this pull request May 18, 2021
@lamkeewei
Merge branch 'develop' into shared-folder
1cfa59d 
* develop:
  chore: upgrade dependencies (#1180)
  Backend tests - updating template for channels (#1175)
  feat(rich-text): support image as links (#1158)
  fix: fix error when updating Telegram ID for an existing phone number (#1178)
  chore: upgrade React; use new JSX transform; sort imports (#1129)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miazima miazima miazima approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lamkeewei @miazima