documentcloud / wordpress-documentcloud

Embed DocumentCloud documents that won't be eaten by the visual editor
https://wordpress.org/plugins/documentcloud/
GNU General Public License v2.0
9 stars 14 forks source link

Only store metadata when post contains an embed #51

Closed m4lvin closed 7 years ago

m4lvin commented 7 years ago

The meta variable wide_assets is added on all posts (even those without documentcloud embeds). Some of my users find this irritating. Could you store this information elsewhere or hide it? Or is it meant to be edited manually?

Alternatively, could you make sure wide_assets is only added to posts actually containing documentcloud embeds?

reefdog commented 7 years ago

@m4lvin Yeah, I can see that being irritating. We inherited this plugin from @argoproject, and they used this boolean to signal some of their themes. Now that the plugin is meant for more generic use, we should revisit the impact of wide_assets.

I'm not caught up on WordPress best practices. What's the better way to store this, and to ensure it's only set if an embed exists? Would you be able to take a crack at it?

@eyeseast or @aschweigert: any idea if the current themes still care about wide_assets?

m4lvin commented 7 years ago

Thanks for the reply. If extra information per post is really needed, then it should be in a separate table managed by the plugin only. And In the post editor add_meta_box seems to be the right thing to use?

I will not implement this myself though, because now I realized that oEmbed without any plugin is enough for our purposes. Then the meta variables should not be generated, correct?

reefdog commented 7 years ago

Correct. If you don't have our plugin installed, we wouldn't have any facility for storing metadata. Glad to hear the vanilla oEmbed works for you!

I'm going to reframe the title of this issue, although I don't know if/when I'll be able to get to it. If you do want to take a crack, PRs are much apprecated. :)

eyeseast commented 7 years ago

Copying over what I wrote before on this:

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.

We had a couple different plugins that implemented wide_assets. That's part of why it's in wp_post_meta instead of another table. I think that was also just the normal pattern in 2011, to put everything in meta fields.

I don't know if anyone is still actively developing that theme, but I know several NPR local stations still use it.

Part of the contract is that the theme should only deal with wide_assets if the flag is present, and do nothing in its absence. In theory, it should be safe to remove, but I'd have to take a look at the original code (from 2011) to be sure.

Hope that helps.

reefdog commented 7 years ago

Very interesting, thanks @eyeseast. Since the oEmbed-delivered embed is responsive anyway, does that make this whole conversation unnecessary?

Also, @m4lvin: I just remembered that without the plugin, it's pretty hard to embed notes. You have to know to transform page-anchor-style note URLs that we expose around the platform (e.g., https://www.documentcloud.org/documents/282753-lefler-thesis.html#document/p15/a42227) to the static URL that actually contains the note-specific oEmbed discovery tag (https://www.documentcloud.org/documents/282753-lefler-thesis/annotations/42227.html). That's one of the features of the plugin.

eyeseast commented 7 years ago

Since the oEmbed-delivered embed is responsive anyway, does that make this whole conversation unnecessary?

For the most part, probably. For the StateImpact sites, we had a pattern where we'd display posts differently on the index page if they had wide_assets (usually showing only a preview, instead of the whole doc viewer). But that may be an edge case that shouldn't concern this plugin.

aschweigert commented 7 years ago

are we ok with just removing this? if so, i'll submit a patch.

knowtheory commented 7 years ago

Yep, a patch would be lovely and let's do it.

reefdog commented 7 years ago

@aschweigert If you happen to have WP 4.8 installed locally, verifying it works on that would be 👍 so we can update the readme. No worries if not.

@knowtheory You need me to help push the patch to the WP plugin repo, or are you setup with that?

knowtheory commented 7 years ago

I am not yet set up to do it!

reefdog commented 7 years ago

You're an admin, so all you need to do is clone (or whatever) the svn repo. I should add a Development section to the readme, but basically:

  1. Make changes
  2. When ready to release a new version, do final testing and housekeeping
    • Update changelog in README.md (for GitHub) and readme.txt (for WordPress Plugin thing)
    • Update version number
  3. Copy documentcloud.php and readme.txt from this repo into SVN repo (at https://plugins.svn.wordpress.org/documentcloud/trunk/)
  4. svn push and after a few minutes, verify the update took
  5. Back here in GitHub, tag the release (e.g., v0.4.4) and bump the version number to the next expected version with -dev suffix, e.g. v0.4.5-dev. Of course this might end up actually being v0.5.0 or something, that's fine. Just to mark an inflection point.
knowtheory commented 7 years ago

Awesome :) Thanks for the instrux.

reefdog commented 7 years ago

For sure. Also just noticed README.md slipped into the svn repo. Feel free to remove it, they use readme.txt.