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

feat: oEmbed implementation #139

Merged
merged 20 commits into from
May 24, 2021
Merged

Conversation

KararTY
Copy link
Contributor

@KararTY KararTY commented May 2, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Old stuff

Unfinished, I feel like this needs more thought put into it.
Should we do it this way?
Should we handle each oEmbed separately maybe?

Lacking:
Custom resolvers for some services like Instagram (Needs a token).
Some services are not in the providers file, instead they offer a "discovery" url in their HTML document metafields, this has to be implemented somehow.

Features:

  • Resolves custom oEmbeds based on resolvers.json file.
  • Support for Instagram & Facebook oEmbed service (Thanks @pajlada)

Full original providers file (Chatterino/api comes with its own modified file):
https://oembed.com/providers.json

@leon-richardt
Copy link
Contributor

Quick overview: https://oembed.com/

@pajlada pajlada marked this pull request as ready for review May 9, 2021 13:43
@pajlada pajlada requested a review from zneix May 9, 2021 13:47
Copy link
Member

@pajlada pajlada left a 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 👍

@KararTY
Copy link
Contributor Author

KararTY commented May 9, 2021

Looks good to me 👍

image

For future reference's sake:
Some oEmbed providers don't provide a lot of information in oEmbed, but provide OpenGraph fields and so here's another idea that could potentially be spun into its own PR/Issue:

we could also combine OG and oE date with a clever model - @leon-richardt

CHANGELOG.md Outdated Show resolved Hide resolved
internal/resolvers/oembed/model.go Outdated Show resolved Hide resolved
internal/resolvers/oembed/facebook.go Outdated Show resolved Hide resolved
internal/resolvers/oembed/load.go Outdated Show resolved Hide resolved
internal/resolvers/oembed/load.go Outdated Show resolved Hide resolved
internal/resolvers/oembed/load.go Outdated Show resolved Hide resolved
internal/resolvers/oembed/load.go Outdated Show resolved Hide resolved
@KararTY KararTY requested review from zneix and pajlada May 17, 2021 16:54
internal/resolvers/oembed/facebook.go Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@pajlada pajlada requested a review from zneix May 23, 2021 14:45
Copy link
Collaborator

@zneix zneix left a comment

Choose a reason for hiding this comment

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

I guess it's all fine unless I've missed something.

@pajlada pajlada merged commit 381d977 into Chatterino:master May 24, 2021
@KararTY KararTY deleted the feature-oembed branch May 24, 2021 17:16
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.

4 participants