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 Image Attachments #2933

Merged
merged 1 commit into from
May 14, 2021
Merged

Fix Image Attachments #2933

merged 1 commit into from
May 14, 2021

Conversation

thienlnam
Copy link
Contributor

@thienlnam thienlnam commented May 14, 2021

Details

Fixed Issues

Fixes #2844

Tests / QA Steps

  1. Upload an image, and make sure it is viewable and that you can click it to open the image modal

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-05-14 at 11 06 18 AM

Mobile Web

Screen Shot 2021-05-14 at 12 04 53 PM

Desktop

Screen Shot 2021-05-14 at 12 05 56 PM

iOS

Screen Shot 2021-05-14 at 12 03 22 PM

Android

Screen Shot 2021-05-14 at 12 08 05 PM

@thienlnam thienlnam requested a review from a team as a code owner May 14, 2021 18:04
@thienlnam thienlnam self-assigned this May 14, 2021
@MelvinBot MelvinBot requested review from francoisl and removed request for a team May 14, 2021 18:05
@roryabraham
Copy link
Contributor

@thienlnam Do you want help testing this? Let's get screenshots for all platforms.

@thienlnam
Copy link
Contributor Author

I was hoping to get localization back but still trying to figure out how to do that with this library. I'm also thinking we can just merge this since it will fix the images and figure out localization later. In that case, I can start testing on other platforms!

@thienlnam
Copy link
Contributor Author

Tested on all platforms and they're working as expected, let's just get this out 👍

@thienlnam thienlnam requested a review from roryabraham May 14, 2021 19:08
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Agreed, let's fix this severe bug and then get localization fixed in a follow-up issue. @thienlnam Can you create an issue that describes the context behind this PR and explains why localization needs to be fixed in RenderHTML.js?

@roryabraham
Copy link
Contributor

All yours @francoisl

Copy link
Contributor

@francoisl francoisl left a comment

Choose a reason for hiding this comment

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

Works well on my side!

@francoisl francoisl merged commit 2050060 into main May 14, 2021
@francoisl francoisl deleted the jack-fixAttachments branch May 14, 2021 19:38
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@thienlnam
Copy link
Contributor Author

I chatted with Marc on how to re-add localization to the attachment title and the solution was to move the localization a layer down. The location where localization was added in RenderHTML.js was just making it so that our HTML renderer package wasn't recognizing img html as an image and never went through the img handling (I'm not familiar with the react-native-render-html library so I'm not totally sure why it was doing that, but I know that removing it made it go through the img handling again). I just went ahead and tagged my other PR #2935 to the original issue as it just adds back Localization

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.46-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chat - Sent images are not displayed in chat
4 participants