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

Add ability to upload images to conversations #555

Merged
merged 12 commits into from
May 17, 2024
Merged

Add ability to upload images to conversations #555

merged 12 commits into from
May 17, 2024

Conversation

sternetj
Copy link
Contributor

Changes

  • Users can add up to 10 images at a time and include text with their message
  • Each image is displayed in its own bubble
  • Made the image uploading library an optional dependency since it might not be used by all consumers

Screenshots

messaging-images.mov

@coveralls
Copy link

coveralls commented May 16, 2024 •
edited

Pull Request Test Coverage Report for Build 9129114448

Details

  • 96 of 103 (93.2%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.4%) to 84.486%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/screens/DirectMessagesScreen.tsx 58 60 96.67%
src/hooks/Circles/usePrivatePosts.ts 37 42 88.1%
Totals Coverage Status
Change from base Build 9128737581: 0.4%
Covered Lines: 3884
Relevant Lines: 4467
💛 - Coveralls

rob-snyder
rob-snyder previously approved these changes May 16, 2024
View reviewed changes
Copy link

@rob-snyder rob-snyder left a comment

Choose a reason for hiding this comment

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

This seems good, though I have little familiarity with the react-native-sdk. Nice job working through the unit tests. It's a PITA when you can't reproduce locally

.github/workflows/release.yml
@@ -16,7 +16,7 @@ jobs:
- uses: actions/checkout@v2
- uses: actions/setup-node@v1
with:
node-version: 14
node-version: 18
Copy link

Choose a reason for hiding this comment

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

Man we still have however many repos on node 16 or earlier. Waiting for AWS to just pull that rug out from under us.

src/hooks/Circles/usePrivatePosts.ts Outdated
Comment on lines 411 to 416
type: {
JPEG: AttachmentType.IMAGE,
JPG: AttachmentType.IMAGE,
PNG: AttachmentType.IMAGE,
GIF: AttachmentType.IMAGE,
}[fileType],
Copy link

Choose a reason for hiding this comment

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

Feel like I'm missing something, couldn't this just be

Suggested change
type: {
JPEG: AttachmentType.IMAGE,
JPG: AttachmentType.IMAGE,
PNG: AttachmentType.IMAGE,
GIF: AttachmentType.IMAGE,
}[fileType],
type: AttachmentType.IMAGE,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're not missing anything. This is a relic of an earlier iteration. I originally was trying to add videos too but it had some issues and I wanted to move on and nobody was asking for video uploads.

rob-snyder
rob-snyder previously approved these changes May 17, 2024
View reviewed changes
@sternetj sternetj requested a review from rob-snyder May 17, 2024 14:45
bruce-glazier
bruce-glazier reviewed May 17, 2024
View reviewed changes
src/screens/DirectMessagesScreen.tsx
}
{...({
// accessoryStyle overrides are not documented but are supported
accessoryStyle: {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's fun.

bruce-glazier
bruce-glazier approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@bruce-glazier bruce-glazier left a comment

Choose a reason for hiding this comment

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

This turned out to be quite the task. Everything looks good to me!

@bruce-glazier
Copy link
Contributor

Only NIT: I feel like the photos would look better without being within a message bubble.

@sternetj
Copy link
Contributor Author

Only NIT: I feel like the photos would look better without being within a message bubble.

I think this is possible. I'll play with it and put a followup PR up if I can get it working well.

@sternetj sternetj merged commit a5cc162 into master May 17, 2024
3 checks passed
@sternetj sternetj deleted the messaging-image-uploads branch May 17, 2024 16:01
@github-actions GitHub Actions
Copy link

🎉 This PR is included in version 12.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Has been released to the package repository (NPM, etc) label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rob-snyder rob-snyder rob-snyder approved these changes

@bruce-glazier bruce-glazier bruce-glazier approved these changes

Assignees
No one assigned
Labels
released Has been released to the package repository (NPM, etc)
Projects
None yet
Milestone
No milestone
4 participants
@sternetj @coveralls @bruce-glazier @rob-snyder