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

Use oEmbed #16

Closed
reefdog opened this issue Apr 14, 2015 · 13 comments
Closed

Use oEmbed #16

reefdog opened this issue Apr 14, 2015 · 13 comments
Assignees

Comments

@reefdog
Copy link

reefdog commented Apr 14, 2015

We're adding an oEmbed service to DocumentCloud, so I'll be adapting this plugin to take advantage of it. The syntax of the shortcodes won't change, so it'll be backwards-compatible.

Goals:

  1. Fetch the embed code from our oEmbed service, natch
  2. Support existing [documentcloud] shortcodes for documents, but prefer [dc-document] going forward to distinguish among target resources; option syntax remains the same
  3. Add new shortcodes support for other resources: notes (Support embedding a single note #4), collections/searches (Support embedding collections/searches #3), and (eventually) pages
  4. Add support for recognizing resource URLs on their own line, as described here; if Try to get on the official WordPress oEmbed whitelist #15 were accomplished, then this would only be enabled for versions of WP prior to that
  5. Use this implementation as a meta guide for future plugin authors

Edited: Abandoned design of separate shortcodes for different resources. Going to instead parse URL and determine resource type from there.

@reefdog
Copy link
Author

reefdog commented Apr 16, 2015

@eyeseast While building this, I ran into the documents and wide_assets post metadata you're saving on save(). Can't figure out what these are used for, though. Help?

reefdog added a commit that referenced this issue Apr 16, 2015
Very simplistic implementation: recognizes raw URLs as supported oEmbeddable objects. Doesn't allow any parameter setting. Also tweaked README a little.

Affects #16
Closes #8
reefdog added a commit that referenced this issue Apr 17, 2015
* Fetch embed codes from the new oEmbed service instead of creating them manually (but leave existing functions in place until we're sure they can be discarded)
* Support both `maxwidth/maxheight` and `height/width` shortcode attrs, but only sends `max*` to the provider
* Nominally backwards-compatible with existing shortcodes and settings (should be tested)
* Piggy-backs on WordPress's internal oembed caching; to disable, change `add_shortcode('documentcloud', array(&$this, 'handle_dc_shortcode_with_caching'));` to `add_shortcode('documentcloud', array(&$this, 'handle_dc_shortcode'));`

Affects #16
Closes #14
reefdog added a commit that referenced this issue Apr 18, 2015
* Create class constants for regularly-modifiable settings (mostly for testing)
* Consolidate cached and non-cached shortcode call and just toggle where it mattesr

Affects #16
@eyeseast
Copy link

Hey @reefdog, @knowtheory can we talk about this at some point? Sounds like you guys are taking this plugin in a new direction and I'm feeling a little out of the loop. Should bring in @aschweigert, too, since INN has been recommending this plugin in Largo.

@eyeseast
Copy link

@reefdog: To your question about wide assets, that was something we did in a lot of the @argoproject / @stateimpact plugins. The basic idea was that plugin could set a post meta variable saying that it was meant for a wider post format (like a full-width document) and a theme could deal with that. It's meant to be a loose contract, hence settings for normal width and full width.

@knowtheory
Copy link
Member

@eyeseast Definitely! Would you prefer to jump on the phone or would posting here be suitable?

@eyeseast
Copy link

Thanks. Posting here is great. I'm on the road this week, which is putting me further behind.

@reefdog
Copy link
Author

reefdog commented Apr 18, 2015

Let's definitely chat, I feel a bit sheepish. I'm new to DocumentCloud, and while Ted gave me some ancestry on the plugin, I had it in my head that we'd already taken over maintenance and I let myself come in like a wrecking ball. Should've kept you guys in the loop much better, mea culpa!

@eyeseast
Copy link

Totally ok. I'm happy to see work being done on this. The sudden flurry of activity just caught me by surprise. I'm back to work in just over a week.

@knowtheory
Copy link
Member

@reefdog i wouldn't sweat it, the responsibility for this one is all on me.

So our general disposition is this (all of this is open for discussion, this is just describing where our heads are at):

Any changes we're making to the plugin should be 100% backwards compatible from a user POV, and any forward changes should add functionality (in this case adding notes and searches). The only additional change we've made thus far is to allow the pasting of document URLs directly in and having them create a default embedding (which we can/should make as sensible as possible).

If one wants to configure any documentcloud related embed code they should use shortcodes.

IMO it'd be nice to be able to use a single [documentcloud] shortcode, and use that as a kind of polymorphic interface based on the type of resource it's been passed (although happy to discuss the pros & cons of that).

From the POV of the internals, our main goal in using the plugin as an oEmbed consumer were to 1) give us a test consumer for the DocumentCloud API, and 2) extract the logic for constructing embed codes out into one location (our API), so that in the event that the structure of our embeds change, the logic for that only needs to be rewritten in the API rather than both in the API and here in the plugin.

@eyeseast: We've got what we need at the moment to test the API, but ultimately it'd be nice to be able to announce the oEmbed endpoint plus an update to the wordpress plugin together. We're happy to hold off for now until we discuss further (esp since we'll want to get this into the plugin repo before doing an announcement). That work for you? :)

cc/ @aschweigert same goes for you! Interested in your opinions :)

reefdog added a commit that referenced this issue Apr 20, 2015
I previously overcomplicated the height/width calculations out of deference to WordPress's internal height/width calculator. Realized the plugin already set its own defaults and thus I could simplify things by starting with that, and maintain better backwards-compatibility too.

* Remove complicated height/width calculations
* Make precedence order clear

Affects #16
reefdog added a commit that referenced this issue Apr 20, 2015
And make it supreme above all other width declarations to be consistent with previous plugin behavior.

Affects #16
reefdog added a commit that referenced this issue Apr 20, 2015
* Adjust `save()` as minimally as necessary to support new function names and the dual-`width/maxwidth` presence
* Add note re: need for new post_metadata key
* Add note re: functions we should be able to remove once tested

Affects #16
reefdog added a commit that referenced this issue Apr 20, 2015
reefdog added a commit that referenced this issue Apr 21, 2015
Was committed accidentally. Not ready to be the default within WP.

Affects #16
@reefdog
Copy link
Author

reefdog commented Apr 21, 2015

Status of the oembed branch:

  • Raw URLs and [documentcloud] shortcodes both generate embeds using oEmbed
  • Previous shortcode attributes supported and defaults respected
  • New attributes added for remaining embed options (container, notes, responsive, etc.)
  • save() function minimally modified (just for compatibility with name changes)
  • get_defaults() and embed_shortcode() left for examination, but replaced with get_default_atts() and handle_dc_shortcode()

I want oEmbed to drop in seamlessly without disrupting existing users. If any of you regular users have time to update to the latest version from the oembed branch and see if anything breaks, I'd be grateful.

reefdog added a commit that referenced this issue Apr 21, 2015
reefdog added a commit that referenced this issue Apr 23, 2015
Supports documents and known note variants.

Affects #16
Closes #4
@reefdog reefdog self-assigned this Apr 28, 2015
@eyeseast
Copy link

I think we can close this now that #21 is merged. Thanks for all your hard work on this @reefdog and @knowtheory.

@reefdog
Copy link
Author

reefdog commented Apr 30, 2015

For sure, thanks for all your patience as we hammered this out!

@knowtheory
Copy link
Member

@eyeseast thanks to you & @aschweigert for your guidance! :)

@anthonydb
Copy link
Member

+1!

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

No branches or pull requests

4 participants