-
Notifications
You must be signed in to change notification settings - Fork 356
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
[CORL-3180]: update to not autoplay tenor gifs in mod queue #4669
Conversation
✅ Deploy Preview for gallant-galileo-14878c canceled.
|
} | ||
|
||
const GiphyMedia: FunctionComponent<Props> = ({ | ||
const GifMedia: FunctionComponent<Props> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the name of this component since it is now used for displaying both Tenor and Giphy gifs.
@@ -133,6 +93,7 @@ export const tenorSearchHandler = | |||
url.searchParams.set("key", apiKey); | |||
url.searchParams.set("limit", `${SEARCH_LIMIT}`); | |||
url.searchParams.set("contentfilter", contentFilter); | |||
url.searchParams.set("media_filter", "gif,nanogif"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this media_filter
to only return the response object formats that we need when we are calling the Tenor api here.
tenorStill: still | ||
tenorVideo: video |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're aliasing these to still
and video
when we consume them from the API client side, do we want to call them TenorMedia > tenorStill
and TenorMedia > tenorVideo
? Seems like a duplication of the naming?
Maybe TenorMedia > still
and TenorMedia > video
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I was trying to keep this consistent with the naming for GiphyMedia
, except that we might not have a still
or video
for some Tenor gifs, so types are different.
if (err.message) { | ||
err.message = err.message.replace(tenant.media.gifs.key, "[Sensitive]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works well to hide the api keys, also, we can use bunyan's pvt
and pub
fields to hide these details if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, nice refactors around the presentation logic too!
What does this PR do?
These changes update so that Tenor gifs are not autoplaying in the mod queue. For Tenor gifs, we are now retrieving the
mp4
andgifpreview
formats from the Tenor api and attaching those to the media embed on the comment, so that they can be used to play/pause the gifs in the mod queue, similar to how Giphy gifs display if enabled.To support any Tenor gifs that have been added since Tenor has been an optional and prior to these changes, this adds a fallback of show/hide Gif, similar to what is seen for Gifs in the comment stream.
I also moved Tenor types out into
common
where Giphy types currently live for consistency and so that we can use them in different places.These changes will impact:
What changes to the GraphQL/Database Schema does this PR introduce?
These changes add
still
,video
,width
, andheight
toTenorMedia
schema, to make it consistent with what Giphy has to enable it to be play/paused in the mod queue.Does this PR introduce any new environment variables or feature flags?
no
If any indexes were added, were they added to
INDEXES.md
?n/a
How do I test this PR?
You can have Tenor enabled for gifs, and add some comments with gifs to the comment stream. Go to the admin and find the comments you added in the moderation queue. See that you can play/pause the gifs and that they are not autoplaying.
Add comments with Tenor gifs on
develop
or another branch without these changes, and see that these gifs (when back on this branch) include a show/hide Gif button that can be toggled. This provides a fallback for any comments with Tenor gifs that don't have a still/video to use for play/pause.Were any tests migrated to React Testing Library?
How do we deploy this PR?