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.

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

#68 gif support #127

Open
wants to merge 21 commits into
base: v1
Choose a base branch
from
Open

Conversation

dilyanmarinov
Copy link
Contributor

@dilyanmarinov dilyanmarinov commented Jul 13, 2019

resolves #68

@tomquirk

Some concerns:

  1. I couldn't emulate giphy functionality completely. The giphy API provides an embed url, which I can pass as a url paramter in the message and it embeds correctly in messenger most of the time, but you have to press play and it's not at all like the real gify. The API also provides a URL for the looping gif, but when I send it as an url element, it doesn't display. I tried looking at the documentation for the facebook chat API, but I don't think the library you're using supports all of it. It would be great if you could give me a hint about how I should pass the looping url so it displays correctly.

  2. I left my API key as a default option. I don't think it's too much hassle and giphy actually encourages registering apps. We should encourage users to configure it with their own API key and not commit to support ourselves, but it's nice to have a default.

  3. Tests for gifs are a bit slow, because you're embedding stuff and I'm wondering if they should be removed entirely.

  4. Should I update the doc, or will you update it after you merge V1?

@tomquirk
Copy link
Collaborator

This looks absolutely fantastic! Awesome work @SUBmarinoff , I'll look at testing this today

@tomquirk
Copy link
Collaborator

tomquirk commented Jul 13, 2019

btw, how did you find the v1 refactor for development?

@dilyanmarinov
Copy link
Contributor Author

@tomquirk

It's gotten a lot better since the last time I contributed. I don't know much about javascript and Nodejs, but it was really easy to figure out what to do. Very well organised, kudos.

@tomquirk tomquirk self-assigned this Jul 15, 2019
@tomquirk
Copy link
Collaborator

nice @SUBmarinoff, good to hear. Shoot me an email at [email protected] when you get a change

@tomquirk
Copy link
Collaborator

Hey @SUBmarinoff not sure what happened in this commit but something went a bit funny here. We will need some of the changes in there

@dilyanmarinov
Copy link
Contributor Author

@tomquirk

I merged the current v1. It had failing tests and I though you had work in progress on there, so decided to revert it. I can squash everything so it just shows as one commit and we have a better view on the diff. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants