Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Adjust plugin code. Split to classes, add factory and hooks
Update readme and add Wistia support
[MOOSE-297] Force wistia autplay on click and change tempalte creation
LayaTaal
left a comment
LayaTaal
left a comment
There was a problem hiding this comment.
@MlKilderkin this is an awesome update. I love the reorganization of code and the new filters you created! I definitely think this makes sense to go to 1.2 or 2.0. Please see my comments. I didn't find any blockers, just some questions and ideas.
| * Plugin URI: https://github.com/moderntribe/tribe-embed | ||
| * Description: A Tribe Embed Plugin. | ||
| * Version: 1.1.0 | ||
| * Version: 1.1.1 |
There was a problem hiding this comment.
What do you think about bumping this to a 1.2 or 2.0 vesion?
| - `$by_provider` — List of hosts returned from `tribe_embeds_allowed_provider_hosts_<slug>` | ||
| - `$provider_class` — current provider class | ||
|
|
||
| #### `tribe_embeds_image_sizes_<slug>` |
There was a problem hiding this comment.
I'm a little confused on the intent and difference in the filter here and below.
There was a problem hiding this comment.
This specific for provider. The below one is global version. In case you want catch all items one by one and not only the specific one
| } | ||
|
|
||
| $image_url = strtok( $response_body[0]->thumbnail->url, '?' ); | ||
| switch ( $resolution ) { |
There was a problem hiding this comment.
Should we set a default in case neither 640 or 320 gets passed in?
There was a problem hiding this comment.
Good catch. Updated
| // Prevent edge case when `$image_data` may have boolean value | ||
| if ( ! is_array( $image_data ) || empty( $image_data ) ) { | ||
| $image_data = []; | ||
| } |
There was a problem hiding this comment.
What causes this edge case? I'm wondering if we can type this earlier or fix it when originally set instead of a separate check.
| * @param array<string,mixed> $block | ||
| * @param string $video_id | ||
| */ | ||
| public function build( array $thumb, array $block, string $video_id, Provider $provider ): string { |
There was a problem hiding this comment.
What do you think about approaching this by separating out markup into template files? I wonder if that might help separate concerns a bit more and allow for easily overriding templates in the future if we want. That might be beyond the scope of this PR but if you think it makes sense as an update, maybe drop a note?
There was a problem hiding this comment.
That's actually a good idea. We might need ticket it somewhere
2 participants
What
This PR refactors the plugin architecture by splitting monolithic code into focused classes with dependency injection, introduces a factory pattern for video providers, and adds proper hook management. The changes improve maintainability and follow WordPress best practices.