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

[CORL-1412] Integrate giphy web SDK #3255

Merged
merged 11 commits into from
Nov 11, 2020
Merged

[CORL-1412] Integrate giphy web SDK #3255

merged 11 commits into from
Nov 11, 2020

Conversation

tessalt
Copy link
Contributor

@tessalt tessalt commented Oct 28, 2020

What does this PR do?

Turns out giphy released a web sdk complete with react components shortly after we built our own custom UI. Theirs is better, but there's a couple tradeoffs.

Pros:

  • slicker UI that will be familiar to users ("masonry" grid)
  • infinite scroll for free
  • easier to adapt UI to mobile

Cons:

What changes to the GraphQL/Database Schema does this PR introduce?

giphy rating and api key are now public

How do I test this PR?

search and select gifs on desktop and mobile

@tessalt tessalt requested a review from wyattjoh October 28, 2020 12:53
package.json Outdated
@@ -136,11 +138,13 @@
"prom-client": "^12.0.0",
"proxy-agent": "^3.1.1",
"querystringify": "^2.1.1",
"react-use": "^15.3.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this library is just being used for a nice useDebounce (imported independently) but if we don't want to add this dep i'm happy to write my own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

These dependencies are great, but can we move them to devDependencies? They aren't used by the server so we don't need to include it in dependencies.

@tessalt tessalt changed the title [CORL-1410] Integrate giphy web SDK [CORL-1412] Integrate giphy web SDK Oct 28, 2020
@kgardnr kgardnr added this to the v6.4.0 milestone Oct 28, 2020
package.json Outdated
@@ -136,11 +138,13 @@
"prom-client": "^12.0.0",
"proxy-agent": "^3.1.1",
"querystringify": "^2.1.1",
"react-use": "^15.3.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

These dependencies are great, but can we move them to devDependencies? They aren't used by the server so we don't need to include it in dependencies.

package.json Outdated
"source-map-support": "^0.5.16",
"stack-utils": "^2.0.1",
"striptags": "^3.1.1",
"tsscmp": "^1.0.6",
"url-regex": "^5.0.0",
"use-resize-observer": "^6.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We actually already have our own resize observer provided by:

import { useResizeObserver } from "coral-framework/hooks";

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this provides a better API though, I'd replace our implementation with the package one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does provide a better API in my opinion, I've replaced our implementation in the one place it was used.

@wyattjoh wyattjoh modified the milestones: v6.4.0, v6.5.0 Oct 29, 2020
@tessalt tessalt requested a review from wyattjoh October 30, 2020 13:54
@wyattjoh wyattjoh changed the base branch from master to next November 9, 2020 15:40
@wyattjoh wyattjoh added the 🚀 merge it! Pull requests that should be merged after status checks pass with a review label Nov 11, 2020
@kodiakhq kodiakhq bot merged commit f6b3cae into next Nov 11, 2020
@kodiakhq kodiakhq bot deleted the spike/giphy-sdk branch November 11, 2020 18:00
@wyattjoh wyattjoh modified the milestones: v6.5.0, v6.6.0 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚀 merge it! Pull requests that should be merged after status checks pass with a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants