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

Giphy integration #5814

Closed

Conversation

panagiotis-c
Copy link
Contributor

@panagiotis-c panagiotis-c commented Mar 27, 2021

First of all, this needs a lot of input from product and design.

Fixes element-hq/element-meta#321 !

Flow

I created a new button in MessageComposer, next to the emoji picker and the sticker picker.

I consume the Giphy API for search results, which should be better than the SDK due to privacy concerns, and possible changes in the future.

I display the GIF search results in an one-column list.

I added an option in labs for showing the GIF picker.

image

I send the GIF as an attachment. Another option is to just send the url, but previews generated by the server have some issues currently and it doesn't work: matrix-org/synapse#1278

TODO

  • add svg icon
  • test in electron
  • test in white/dark themes
  • pick correct, small enough filesize to send
  • convert list to two-column?
  • disable confirmation dialog
  • add option in labs
  • add "powered by Giphy"
  • show trending gifs when search box is empty?
  • (for core devs) create api key from https://developers.giphy.com/dashboard/

PR for documentation in labs.md: element-hq/element-web#16808

Related issue: element-hq/element-web#2693

Signed-off-by: Panagiotis Chalimourdas [email protected]


Here's what your changelog entry will look like:

✨ Features

Preview: https://613dd49f7161e91e7bfbfbbf--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

create a new menu in MessageComposer, next to emoji picker and sticker picker

consume the Giphy API

display the GIF search results in an one-column list

add option in settings for showing the GIF picker
@panagiotis-c panagiotis-c marked this pull request as draft March 27, 2021 20:00
@SimonBrandner
Copy link
Contributor

First off, this is awesome :D

Fixes #415

add option in labs?

I am pretty sure that will be required

add showConfirmationDialogs option to sendContentListToRoom
so we can use the same function for directly uploading GIFs
without displaying a confirmation dialog every time
add translations
show mp4 version in preview, for smaller file sizes
better handling for throttling requests and lazy loading list
cancel promises on unmount
@ShadowJonathan
Copy link

Why is this in matrix-react-sdk? I see this purely as an addition to matrix in the regards of Element, so this should (in my opinion) only be implemented from element-web.

I think there's a bit of a separation-of-concerns issue here.

@jryans
Copy link
Collaborator

jryans commented Mar 30, 2021

Why is this in matrix-react-sdk? I see this purely as an addition to matrix in the regards of Element, so this should (in my opinion) only be implemented from element-web.

I think there's a bit of a separation-of-concerns issue here.

Hmm, it's a bit unclear perhaps. I think we should first review the product fit here, and from there we can work out the proper home for the code.

@jryans jryans requested review from a team March 30, 2021 10:30
@kojid0
Copy link

kojid0 commented Mar 31, 2021

Why not use the sidebar? There is enough space to display many things more clearly than in a small window. This also applies to emoji and stickers... GIFs could be the first to support sidebar accesibility, with Emoji and Sticker implemented later.

111 (Custom)

@SimonBrandner
Copy link
Contributor

Why not use the sidebar? There is enough space to display many things more clearly than in a small window. This also applies to emoji and stickers... GIFs could be the first to support sidebar accesibility, with Emoji and Sticker implemented later.

This wouldn't match the current design... So if this change were to be implemented, it should in bulk with emojis and stickers. This is ultimately up to the design team to decide. I remember there being an issue about this, but I can't find it

@panagiotis-c
Copy link
Contributor Author

panagiotis-c commented Mar 31, 2021

There is this issue element-hq/element-web#16180 and this one element-hq/element-web#2693 (comment)

If we keep it like it is right now (without the sidebar), we can also make the popup panel bigger and have two columns of gifs.

@tsukasagenesis
Copy link

nice feature ! can't wait to get it integrated !

import { DebouncedFunc, throttle, uniqBy } from "lodash";
import { Gif } from "./Gif";

const API_KEY = "hhMBhTnD7k8BgyoZCM9UNM1vRsPcgzaC";

Choose a reason for hiding this comment

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

This probably shouldn't be checked into git.
Unrelated though, this work is amazing :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

This is a public key so it is not a security issue, as the request will be done client-side and anyone can inspect it.
Giphy/giphy-js#120 (comment)

If this is approved by the product team a production key is needed in order to increase the limits by Giphy.

@t3chguy t3chguy removed request for a team July 1, 2021 07:24
@t3chguy
Copy link
Member

t3chguy commented Jul 1, 2021

Removing review as PR is a draft

@panagiotis-c
Copy link
Contributor Author

Removing review as PR is a draft

@t3chguy this needs feedback by the product team in order to continue development, so should I mark it as ready for review?

@t3chguy
Copy link
Member

t3chguy commented Jul 1, 2021

Yes, draft means still work in progress, not ready for review.

image

@panagiotis-c panagiotis-c marked this pull request as ready for review July 1, 2021 13:46
@panagiotis-c
Copy link
Contributor Author

panagiotis-c commented Jul 1, 2021

I will wait for feedback, then, before proceeding with improvements because the feature may not be desired in this place in the codebase.

@t3chguy t3chguy requested review from a team July 1, 2021 13:54
@erAck
Copy link

erAck commented Jul 16, 2021

If this hits product then please

  • make it a preference option
  • have it disabled per default
  • have it disabled per default for recipients (i.e. do not display the gif unless enabled)
  • finally have a way to fold away images in rooms

Reason is that animated gifs are utterly distracting, easily get on the nerves and unnecessarily consume CPU and power .
Also, images in general take too much space in the timeline, which the last bullet point would take care of.

Additionally, Giphy is privacy invading as the client's IP and search ends up with Facebook that as we all know will collect everything it can get its hands on.

I actually wish this will never get implemented.

@seanfarley
Copy link

If this hits product then please

  • make it a preference option
  • have it disabled per default
  • have it disabled per default for recipients (i.e. do not display the gif unless enabled)
  • finally have a way to fold away images in rooms

Reason is that animated gifs are utterly distracting, easily get on the nerves and unnecessarily consume CPU and power .
Also, images in general take too much space in the timeline, which the last bullet point would take care of.

Additionally, Giphy is privacy invading as the client's IP and search ends up with Facebook that as we all know will collect everything it can get its hands on.

I actually wish this will never get implemented.

Tell me you're a toxic dev without telling me you're a toxic dev.

@MadLittleMods MadLittleMods added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Aug 24, 2021
@DoM1niC
Copy link

DoM1niC commented Sep 4, 2021

@panoschal can you fix the conflicts PLZ :) I wanne use this in my Fork

@panagiotis-c panagiotis-c requested a review from a team as a code owner September 12, 2021 10:10
@panagiotis-c
Copy link
Contributor Author

I fixed the conflicts @DoM1niC

@DoM1niC
Copy link

DoM1niC commented Sep 12, 2021

Nice and THX I will try it out soon 😊😊

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This is quite a large chunk of functionality that, as you noted, needs design + product review. I'm mostly leaving this comment to clear the automatic code review request that github will insist on while we wait for review (given code review on something this size before design looks at it can be detrimental to progress).

Something I did notice is that the new files are missing copyright/license headers - these should be added to ensure everything is all squared away :)

@nadonomy nadonomy assigned nadonomy and gaelledel and unassigned nadonomy Oct 19, 2021
Copy link

@gaelledel gaelledel left a comment

Choose a reason for hiding this comment

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

Hi there, thank you for your work on this!

I have reviewed it and would like to propose the following changes available on Figma here https://www.figma.com/file/ifIvZA4XIP1JTyndHFr5Ub/?node-id=10%3A7689

I have added behaviour specs for you but if you have any additional question, just ping me here and I'll get back to you.

@panagiotis-c
Copy link
Contributor Author

Great, I will try to implement these improvements soon.

@gaelledel
Copy link

@panoschal Thanks a lot for helping out on this!
Just to let you know that we're actually having more conversations around technical feasibility of this internally so I'd suggest to hold off until we get those clarifications sorted.
@t3chguy @jakewb-b FYI

@DoM1niC
Copy link

DoM1niC commented Nov 5, 2021

Great, I will try to implement these improvements soon.

needs threads support the Composer isn't upload the gif there... :(

update the fully Merge is broken after last Commits in ContentMessages.tsx

@nnuel
Copy link

nnuel commented Dec 19, 2021

Just tested, I like the uploading feature, and would also second the two-column lay-out.

@williamkray
Copy link

Rather than including the giphy api key in the code, this should be something users can fill in, or a config value pulled from the element-web config.json.

If the value is not supplied, the feature should be hidden/nonfunctional.

@arkitoure
Copy link

arkitoure commented Mar 6, 2022

What's the status of this integration? Just throwing this out into the ether...how about selectable support for other gif platforms?

  • tenor.com
  • gifer.com
  • gfycat.com
  • duckduckgo gif search

Beyond tenor, this could address privacy concerns for some?

@floviolleau
Copy link

Hi,

Any news on this awesome integration ready to fly?

Thanks!

@iceHtwoO
Copy link

iceHtwoO commented May 27, 2022

Hi,
due to this PR not being updated, I have started developing a solution myself.
It should be ready in a few days.
Screenshot 2022-05-27 141323

https://github.com/iceHtwoO/matrix-react-sdk/tree/feature/gifpicker_integration

@panagiotis-c
Copy link
Contributor Author

Hello, @gaelledel you mentioned that there were technical considerations around this, did you have any update?

@panoschal Thanks a lot for helping out on this! Just to let you know that we're actually having more conversations around technical feasibility of this internally so I'd suggest to hold off until we get those clarifications sorted. @t3chguy @jakewb-b FYI

@iceHtwoO iceHtwoO mentioned this pull request May 28, 2022
7 tasks
@turt2live
Copy link
Member

Thank you for working on this, however per element-hq/element-meta#321 (comment) I don't think we can safely keep this sort of change open. The feature requires design and product review and is generally quite large in size - we might try to take the work on ourselves if/when product/design get a chance to take a look.

@turt2live turt2live closed this May 28, 2022
@khazaddum
Copy link

I conviced my family to switch to a Matrix homeserver and Element, and their first question was "how do I send GIFs?"

@CRCinAU
Copy link

CRCinAU commented Feb 13, 2023

Did this ever get anywhere?

Or has it become a wasteland where PRs go to die?

@iceHtwoO
Copy link

Did this ever get anywhere?

Or has it become a wasteland where PRs go to die?

This is sadly going no where, you can find more information in the following issue. There have been multiple attempts to integrate the feature.

@WesLee91
Copy link

WesLee91 commented Sep 29, 2023

Anyone know of any Matrix client that has giphy integration?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for a GIF keyboard / ability to easily send GIFs