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: replace static video thumbnails with actual thumbnails #1439

Merged
merged 15 commits into from
Jul 7, 2022
Merged

feat: replace static video thumbnails with actual thumbnails #1439

merged 15 commits into from
Jul 7, 2022

Conversation

khushal87
Copy link
Member

🎯 Goal

This PR replaces the static black thumbnail with the thumbnail generated from the Backend.

🛠 Implementation details

The thumb_url field is added to the VideoThumbnail component and the View is replaced by ImageBackground having the thumb_url.

🎨 UI Changes

iOS
Before After
Android
Before After

🧪 Testing

☑️ Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Commits follow the conventional commits spec
  • New code is covered by tests
  • Screenshots added for visual changes
  • Documentation is updated

@khushal87
Copy link
Member Author

Reviewers,
The build is failing because of the missing prop definition in the SendFileAPIResponse where we need to add the thumb_url property. I have already raised a PR for this and it is merged. Let's wait for the release.

PR - GetStream/stream-chat-js#982

@khushal87
Copy link
Member Author

Note: The video thumbnail will only be visible for new videos uploaded to the CDN. The videos uploaded previously won't show the thumbnail.

@khushal87 khushal87 marked this pull request as draft June 3, 2022 11:09
vanGalilea
vanGalilea approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@vanGalilea vanGalilea left a comment

Choose a reason for hiding this comment

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

The draft looks good to me, pretty straightforward!

@vanGalilea
Copy link
Contributor

vanGalilea commented Jun 8, 2022 •
edited
Loading

@khushal87 can you please resolve the failing check pr step? And please let's resolve the conflicts

@khushal87
Copy link
Member Author

khushal87 commented Jun 8, 2022 •
edited
Loading

The reason check fail is because stream-chat-js needs a release mentioned above in the comment. I will check the conflicts. @vanGalilea

@vanGalilea
Copy link
Contributor

@khushal87 we should expedite this one 😳

@khushal87 khushal87 marked this pull request as ready for review June 23, 2022 05:16
vanGalilea
vanGalilea suggested changes Jul 5, 2022
View reviewed changes
@vanGalilea
Copy link
Contributor

@khushal87 can you please add test steps on how to test this feature locally?

@khushal87 khushal87 requested a review from vanGalilea July 6, 2022 11:47
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

83.3% 83.3% Coverage
0.0% 0.0% Duplication

@vanGalilea vanGalilea merged commit 12caa9d into develop Jul 7, 2022
@vanGalilea vanGalilea deleted the khushal87-issue-1236 branch July 7, 2022 09:18
@vishalnarkhede
Copy link
Contributor

🎉 This PR is included in version 4.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vanGalilea vanGalilea vanGalilea approved these changes

@vishalnarkhede vishalnarkhede Awaiting requested review from vishalnarkhede

@madsroskar madsroskar Awaiting requested review from madsroskar

@santhoshvai santhoshvai Awaiting requested review from santhoshvai

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

Successfully merging this pull request may close these issues.
3 participants
@khushal87 @vanGalilea @vishalnarkhede