Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

inline playback for gif attachments #326

Merged
merged 3 commits into from
Dec 18, 2017
Merged

inline playback for gif attachments #326

merged 3 commits into from
Dec 18, 2017

Conversation

csduarte
Copy link
Contributor

Summary

Adds Inline playback for git attachments

Cc @dmeza

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Ran make check-style to check for style errors (required for all pull requests)
  • Ran make test to ensure unit and component tests passed
  • Added or updated unit tests (required for all new features)
  • Has server changes (please link)
  • Has redux changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json) updates
  • Touches critical sections of the codebase (auth, posting, etc.)

@jasonblais jasonblais added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Nov 20, 2017
@esethna esethna removed the Setup Old Test Server Triggers the creation of a test server label Nov 27, 2017
@mattermost mattermost deleted a comment from mattermod Nov 27, 2017
@mattermost mattermost deleted a comment from mattermod Nov 27, 2017
@mattermost mattermost deleted a comment from mattermod Nov 27, 2017
@esethna esethna added the Setup Old Test Server Triggers the creation of a test server label Nov 27, 2017
esethna
esethna suggested changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

@dmeza please help fix the build issueswhen you have a chance so we can start up the test server

@esethna esethna added the Awaiting Submitter Action Blocked on the author label Nov 27, 2017
@dmeza dmeza force-pushed the BASH-6_inline_gif_playback branch 2 times, most recently from 01c306f to 7ca008c Compare November 28, 2017 18:00
@dmeza
Copy link
Contributor

dmeza commented Nov 28, 2017

@esethna rebased to latest from master, added gif playback to preview and re-created snapshots. Checked that styles and tests are passing.

@esethna esethna removed the Setup Old Test Server Triggers the creation of a test server label Nov 28, 2017
@mattermost mattermost deleted a comment from mattermod Nov 28, 2017
@mattermost mattermost deleted a comment from mattermod Nov 28, 2017
@mattermost mattermost deleted a comment from mattermod Nov 28, 2017
@esethna esethna added the Setup Old Test Server Triggers the creation of a test server label Nov 28, 2017
@mattermod
Copy link
Contributor

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

@mattermod
Copy link
Contributor

Spinmint test server created at: http:https://i-08910b2a8dcfb8c75.spinmint.com

Test Account 1: Email: [email protected] | Password: passwd

Test Account 2: Email: [email protected] | Password: passwd

Instance ID: i-08910b2a8dcfb8c75

esethna
esethna approved these changes Nov 29, 2017
View reviewed changes
Copy link
Contributor

@esethna esethna left a comment

Choose a reason for hiding this comment

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

Cool thanks @dmeza!

Heads up that we have an incoming PR for updating image thumbnails. I want to make sure these changes are not going to conflict?

@dmeza
Copy link
Contributor

dmeza commented Nov 29, 2017

@esethna reviewed the PR and the conflict will be on the snapshot for components/file_attachment.jsx. Otherwise it should be fine.

@esethna esethna added 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager Awaiting Submitter Action Blocked on the author Setup Old Test Server Triggers the creation of a test server labels Nov 30, 2017
@mattermod
Copy link
Contributor

Spinmint test server destroyed

@esethna esethna removed their assignment Nov 30, 2017
@esethna esethna added this to the v4.5.0 milestone Dec 5, 2017
@dmeza
Copy link
Contributor

dmeza commented Dec 7, 2017

@hmhealey thanks for the comments.

  • Since there's no way to know if it's animated or not, it does load the full image in for gifs. Do you know of a way we can control that?
  • It would need a config to display the thumbnail, but that could be a following step.

@hmhealey
Copy link
Member

hmhealey commented Dec 7, 2017

For whether or not it's an animated gif, I believe the FileInfo object will have has_preview_image set to false if it's an animated gif (to prompt it to show the animated gif in the preview modal

For turning on/off the animation, I'll double check with PMs. When we had something similar to it in the past, we had a toggle like that, but maybe it's not necessary any more

hmhealey
hmhealey suggested changes Dec 7, 2017
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Oh wait, I forgot that I still wanted the thumbnail version to be used for non-animated gifs

@dmeza
Copy link
Contributor

dmeza commented Dec 7, 2017

@hmhealey I'll add that change later today.

@esethna esethna modified the milestones: v4.5.0, v4.6.0 Dec 8, 2017
@dmeza dmeza force-pushed the BASH-6_inline_gif_playback branch 2 times, most recently from bddec4c to a76b36f Compare December 8, 2017 17:01
@dmeza
Copy link
Contributor

dmeza commented Dec 8, 2017

@hmhealey made the fix to check than when it's a gif and has_preview_image is true to display the thumbnail. Rebased to latest from master.

hmhealey
hmhealey approved these changes Dec 18, 2017
View reviewed changes
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks, looks good

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Dec 18, 2017
@hmhealey hmhealey merged commit 6c74c85 into mattermost:master Dec 18, 2017
saturninoabril added a commit that referenced this pull request Dec 18, 2017
@saturninoabril
Revert "inline playback for gif attachments (#326)"
9706f90 
This reverts commit 6c74c85.
@lindalumitchell lindalumitchell added the Tests/Done Release tests have been written label Dec 29, 2017
@jasonblais jasonblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Jan 4, 2018
dmeza pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Jan 9, 2018
@csduarte
inline playback for gif attachments (mattermost#326)
86cd560 
* inline playback for gif attachments

* Re-created snapshot for file_attachment.test

* Display thumbnail for non animated gifs
dmeza pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Jan 15, 2018
@csduarte
inline playback for gif attachments (mattermost#326)
810392c 
* inline playback for gif attachments

* Re-created snapshot for file_attachment.test

* Display thumbnail for non animated gifs
dmeza pushed a commit to uber-archive/mattermost-webapp that referenced this pull request Jan 15, 2018
@csduarte
inline playback for gif attachments (mattermost#326)
6979fc7 
* inline playback for gif attachments

* Re-created snapshot for file_attachment.test

* Display thumbnail for non animated gifs
@dmeza dmeza deleted the BASH-6_inline_gif_playback branch December 7, 2018 18:28
hmhealey pushed a commit that referenced this pull request Aug 28, 2020
@jespino @hmhealey
Use remote-redux-devtools only when browser extension is not available (
c412089 
#326)
hmhealey pushed a commit that referenced this pull request Mar 17, 2021
@jespino @enahum
Use remote-redux-devtools only when browser extension is not available (
e2b6126 
#326)
@giuesposbit giuesposbit mentioned this pull request Feb 3, 2024
Open
@DSchalla DSchalla mentioned this pull request Feb 3, 2024
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jwilander jwilander jwilander approved these changes

@hmhealey hmhealey hmhealey approved these changes

@esethna esethna esethna approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
Milestone
v4.6.0
9 participants
@csduarte @dmeza @mattermod @hmhealey @jwilander @esethna @jasonblais @lindalumitchell @miguelespinoza