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

Include videos by default #44

Closed
nfriedly opened this issue Sep 19, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@nfriedly
Copy link
Collaborator

commented Sep 19, 2018

I think that this plugin should include the video in the post by default when it imports from instagram. I know https://gist.github.com/jtsternberg/b7c3b5371c6f639693b8f086859ad129 exists, but I feel like that shouldn't be necessary, and it doesn't give any straightforward way to stick the video into e.g. the middle of the post.

I think we should add some new tags like **insta-video** and maybe a **insta-video-or-image** that would insert one or the other but not both.

Additionally it might make sense to have content-type conditionals such as [if-type-image] and [if-type-video] (and, eventually, an [if-type-carousel] - see #43).

Or, alternatively, different templates for different types.

@nfriedly

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 21, 2018

Hey, @jtsternberg: I've been thinking about this for a couple of days, and I wanted to hear your thoughts before I go to far on an implementation.

To fully support videos and carousels in the template, I think we'd need these additions:

  • New conditionals: [if-type-image], [if-type-video], and [if-type-carousel]
  • A new **insta-video** tag that becomes an empty string if there isn't a video
  • A new type of tag for looping over carousel items, something like [foreach-carousel-item]...[/foreach-carousel-item] and then inside of that tag:
    • The contents would be repeated for each carousel item
    • **insta-image**, **insta-video**, [if-type-image], and [if-type-video] would apply to the current carousel item rather than the overall post.

The default template would then look something like this:

[if-type-image]<p><a href="**insta-link**" target="_blank">**insta-image**</a></p>[/if-type-image]
[if-type-video]**insta-video**[/if-type-video]
[if-type-carousel][foreach-carousel-item]
  [if-type-image]<p><a href="**insta-link**" target="_blank">**insta-image**</a></p>[/if-type-image]
  [if-type-video]**insta-video**[/if-type-video]
[/foreach-carousel-item]
<p>**insta-text**</p>
[if-insta-location]<p>Photo taken at: **insta-location**</p>[/if-insta-location]
<p><a href="**insta-link**" target="_blank" class="instagram-link">View in Instagram ⇒</a></p>

However, this is starting to get a little complex, and I'm starting to think that we've reached the point where it might make sense to just use a third-party templating language & library instead of implementing all of the above ourselves. I don't have any particular one in mind, and I don't have a strong opinion on it; what do you think?


Oh, and one other thing that crossed my mind and is vaguely related to this: It'd be nice to provide the ability to re-apply an updated template to existing posts. I think we'd need to save the original instagram text in a meta field, and the order of the attachments for carousel posts. Is there anything else we'd need?

@nfriedly

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 21, 2018

One more followup thought: perhaps we could simplify [foreach-carousel-item] to just [foreach-item] and let it run as a "loop" over the single item on image/video posts.

That would simplify the default template a bit by allowing the it to skip the first two lines and the condition on the third line:

[foreach-item]
  [if-type-image]<p><a href="**insta-link**" target="_blank">**insta-image**</a></p>[/if-type-image]
  [if-type-video]**insta-video**[/if-type-video]
[/foreach-item]
<p>**insta-text**</p>
[if-insta-location]<p>Photo taken at: **insta-location**</p>[/if-insta-location]
<p><a href="**insta-link**" target="_blank" class="instagram-link">View in Instagram ⇒</a></p>

I think that certainly feels a little more friendly and DRY.

We could still keep the conditionals for more advanced usage, but I think this feels like a better starting place. (I think this also makes sense with a third-party templating library.)

@nfriedly

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 21, 2018

Oh, and it just occurred to me that you got hit by a hurricane last week. Sorry for hitting you with a wall of stuff in that context, this all can wait.

@jtsternberg

This comment has been minimized.

Copy link
Owner

commented Oct 18, 2018

I like your thoughts on this, but will introduce more mental overhead when trying to setup the importer settings. If we implement this, I think it would likely be best if we setup a wiki on this repo and link to it from the plugin for help with advanced options. Honestly, this is something I should do either way.

Let me know if you'd like to submit a PR for this.

@nfriedly

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 22, 2018

Yea, I started working on this with twig 1.x (because 1.x supports all of the old versions of PHP that wordpress does), but I don't have it in a working state yet.

I think that's probably going to be the way to go, if it proves too difficult (or to have many conflicts with other WP modules), then I'll drop twig and work on improving the hand-rolled template system.

jtsternberg added a commit that referenced this issue Oct 23, 2018

Add media-type conditions to output different things depending on the…
… type of media from instagram. Related: #44
@jtsternberg

This comment has been minimized.

Copy link
Owner

commented Oct 23, 2018

I'm still up for twig-type things, but I couldn't wait as I needed the media-type conditionals for my own blog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.