-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feature: Include images & captions into export #117
Conversation
…/images-captions-export-ben
Gonna open a separate PR for the PHP Standards Scrutinizer failures. Getting lots unrelated to these code changes. |
@bmarshall511 As you'll be opening another PR to address coding standards changes, is it possible to limit this PR to just the functional changes? That will aid with code review and keep the changelog more accurate. |
…/images-captions-export-ben
9caa6f2
to
22b7163
Compare
@peterwilsoncc No problem! I've reverted back to the functional changes (minus my IDE auto-fixing some of the coding standard issues in the touched files) and merged |
includes/functions/articles.php
Outdated
$this->items[] = $post; | ||
} | ||
} | ||
|
||
/** | ||
* Generates content for a single row of the table | ||
* | ||
* @param object $item The current item | ||
* @param object $item The current item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be an incorrect PHPCS fix because the line below is incorrect (it has the param name first and the type second and those should be switched)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
includes/functions/articles.php
Outdated
@@ -260,7 +275,7 @@ function _column_title( $item, $classes = '', $data = '', $primary = false ) { | |||
*/ | |||
function column_cb( $item ) { | |||
return '<input type="checkbox" class="article-status" name="article-status[]" value="' . | |||
( isset( $item->ID ) ? absint( $item->ID ) : '' ) . '" />'; | |||
( isset( $item->ID ) ? absint( $item->ID ) : '' ) . '" />'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, line 292, 298 and 303, I get these are automated PHPCS fixes but not sure we should even have extra line breaks here to begin with? I'd almost just delete the line break and keep everything on one line, to avoid having extra whitespace show in these inputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
* Builds the file name for the zip | ||
* Uses the print issue title & day/time | ||
* | ||
* @uses get_timezone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't seem like this comment is accurate as this method isn't used here but in get_zip_file_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
'post_status' => __( 'Article Status', 'eight-day-week' ), | ||
); | ||
|
||
$title_offset = array_search( 'title', array_keys( $columns ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like some extra spacing here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
||
$content = $this->article->post_content; | ||
|
||
if ( $post = get_post( get_post_thumbnail_id( $this->id ) ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move the assignment of a variable outside of the if
statement:
if ( $post = get_post( get_post_thumbnail_id( $this->id ) ) ) { | |
$thumbnail = get_post( get_post_thumbnail_id( $this->id ) ); | |
if ( $thumbnail ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
$content = $this->article->post_content; | ||
|
||
if ( $post = get_post( get_post_thumbnail_id( $this->id ) ) ) { | ||
if ( $image = $this->get_image_name( $post->ID ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
} | ||
} | ||
|
||
$content = preg_replace_callback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing a number of regex parsing here and this may be fine but parsing HTML using regex can be flaky. May want to consider using something like DOMDocument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
// return $image ? $this->get_image_tag($image[1], $caption) : ''; | ||
return ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, seems like we never return the image since that line is commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( $featured_id = get_post_thumbnail_id( $this->id ) ) { | ||
if ( $image = $this->get_image_name( $featured_id ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above comment on assigning a variable within an if
statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
I've not tested things out yet but did an initial review. I wouldn't be surprised if most of these are pre-existing issues but seems like a good time to fix those up if we're going to be touching those lines of code anyway. Let me know if there's any questions or concerns on that though. |
…eight-day-week into feature/images-captions-export-ben
Probably good to resolve the items Darin called out here and then any additional ones can be handled in #120 |
… standards so they can be addressed in another pull request
@dkotter I've updated the code based on your comments & reverted the coding standard updates that were unrelated to this issue. Sorry for the confusion, as I got deeper, realized that would be better to move those updates to their own PR due to the massive amount instead on muddying up this one with a bunch of changes unrelated to the functionality of this PR. With that being said, I'm not super familiar with this plugin or using the exported XML in programs like InDesign, but based on what I'm seeing in the export files, it all seems to be there. In my testing. worked with single images, gallery images & included captions. Thanks for checking it out! Let me know if there's anything else I need to hit. |
Description of the Change
New feature to include images & captions in the export file, see #22
Closes #22
How to test the Change
Changelog Entry
Credits
Props @bmarshall511
Checklist: