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

Adds examples of oembed parameters #38

Merged
merged 19 commits into from
Mar 18, 2019

Conversation

kennethormandy
Copy link
Contributor

Hi, thanks for your work on this plugin, it’s working really well for me!

Before I went too far with this, I was wondering what you thought about adding support for configuring individual oEmbed providers? As far as I could tell, this isn’t possible with the current plugin, although if I’ve missed something just let me know.

I’ve hard-coded a Twitter example in helpers.js just as an easy way to try this idea out, but I would expect you would manually do something like this in your gatsby-config.js:

// …
{
  resolve: `@raae/gatsby-remark-oembed`,
  options: {
    providers: {
      include: [
        {
          name: 'Twitter', // Maybe resolve instead of name?
          options: {
            theme: 'dark' // Use the Twitter dark theme
            dnt: true // Do not track
            hide_media: true // Hide Twitter card media
          }
        },
        'Instagram'
      ],
    },
  },
}
// …

On Twitter, this will let you configure some of the parameters you would normally change through data-* attributes, which of course don’t make sense here, because the HTML is generated for you. Another API I could imagine:

https://twitter.com/kennethormandy/status/1234?theme=dark

Or related to #23:

oembed:https://twitter.com/kennethormandy/status/1234?theme=dark

Presumably, this would let you configure the settings for each individual embed, if you wanted something different between them. Personally, my use case would be solved with the global settings in gatsby-config.js, but I could see this being useful too.

Looking forward to hearing what you think.

@raae
Copy link
Member

raae commented Feb 28, 2019

I am liking this.

Let's start with adding it to the config and let it be a global setting.

We can add the possibility of overriding settings on a link later.

Remember to update docs and add tests as you see fit.

@raae
Copy link
Member

raae commented Mar 11, 2019

Let me know if you are stuck in any way (other than lack of time, can't help you there...).

@kennethormandy
Copy link
Contributor Author

@raae Thanks for the prompt, I have been meaning to reach out because I did get stuck! I don’t have a lot of experience with async/await yet, so I am probably making some mistakes.

I pushed the earlier commits, and I added some better tests, but still tripping up here:

  • The tests seem to return the proper data, but running in on a Gatsby project with npm link gives me GraphQLError: Type providers must define one or more fields.
  • One #getProviderEndpointUrlForLinkUrl test is failing now, I think because ammendProviders modifies the providers array, and I am testing Instagram earlier. I can probably fix this one if I properly copy the providers array for the ammendProviders only.

You can use it with the regular config and everything works as normal.

@raae
Copy link
Member

raae commented Mar 12, 2019

I will have a look at it tomorrow morning, I am on GMT+1 (Oslo) time.

@raae
Copy link
Member

raae commented Mar 13, 2019

Hey, I made it work!

You can see it in my kennethormandy-ko-config-demo branch. If you give me access to your fork I can push it there and we keep it all in this pull request.

I moved the added settings out of include as one might want to add these settings without limiting the provider list.

So your example config would now be:

        plugins: [
          {
            resolve: `gatsby-remark-oembed`,
            options: {
              providers: {
                settings: {
                  Twitter: {
                    theme: 'dark', // Use the Twitter dark theme
                    dnt: true, // Do not track
                    hide_media: true, // Hide Twitter card media
                  }
                }
              }
            }
          }
        ]
      }

@kennethormandy
Copy link
Contributor Author

Great thinking, that solves both problems. I didn’t think about someone wanting to keep all the default providers and still add settings, that’s a good change.

I gave you access to the fork, but I will also try cherry picking your commit, and then I can update the docs.

@raae
Copy link
Member

raae commented Mar 13, 2019

Great, cherry-pick away. I have time this weekend to look over final code and merge :D

@raae
Copy link
Member

raae commented Mar 17, 2019

Got the flu and ome work stuff getting in the way. Will hopefully get to is soon.

@kennethormandy
Copy link
Contributor Author

kennethormandy commented Mar 17, 2019

@raae Thanks for letting me know, and hope you feel better quickly!

I’m happy using the fork with your updates for now, and can easily switch to a new version when you have a more convenient block of time to review this.

@raae raae merged commit f50fae2 into queen-raae:master Mar 18, 2019
@raae
Copy link
Member

raae commented Mar 18, 2019

Woke up too early, so this happened 👍

@raae
Copy link
Member

raae commented Mar 18, 2019

Send me your address on twitter DM (@raae) and I'll hook you up with some Norwegian chocolate 🍫

@raae
Copy link
Member

raae commented Mar 24, 2019

Just a reminder of the above @kennethormandy 🍫 📫

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.

2 participants