-
Notifications
You must be signed in to change notification settings - Fork 10
Snapshot merge #92
Snapshot merge #92
Conversation
php/class-post-type.php
Outdated
| } ); | ||
|
|
||
| foreach ( $posts as $post ) { | ||
| $post->post_content = $this->get_post_content( $post ); |
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.
I think the data parsed from the post_content should be put into another array instead of stuffed back into $post. So maybe:
$snapshots_data = array();
foreach ( $posts as $post ) {
$snapshots_data[] = $this->get_post_content( $post );
}And use $snapshots_data instead of $content.
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.
I have changed that, I kept it as such because I thought we will need mapping of post and data. However, I never used that mapping, but now to add uuid in merge_conflcit i will use it,
Creating another array like
$snapshot_post_data = array();
foreach ( $posts as $post ) {
$snapshot_post_data[] = array(
'data' => $this->get_post_content( $post ),
'uuid' => $post->post_name,
);
}
php/class-post-type.php
Outdated
| } | ||
| $content = wp_list_pluck( $posts, 'post_content' ); | ||
| $conflict_keys = call_user_func_array( 'array_intersect_key', $content ); | ||
| $merge_value = call_user_func_array( 'array_merge', $content ); |
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.
Better name than $merge_value would be $merged_snapshot_data.
| add_action( 'admin_print_scripts-revision.php', array( $this, 'disable_revision_ui_for_published_posts' ) ); | ||
|
|
||
| // Version check for bulk action. | ||
| if ( version_compare( get_bloginfo( 'version' ), '4.7', '>=' ) ) { |
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.
Nice. Better than using the $wp_version global which I usually use. I'll do this in the future.
php/class-post-type.php
Outdated
| $original_values[] = $post->post_content[ $key ]; | ||
| } | ||
| } | ||
| array_pop( $original_values ); |
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.
Maybe leave the winner in the merge conflict intact?
php/class-post-type.php
Outdated
| } | ||
| } | ||
| array_pop( $original_values ); | ||
| $merge_value[ $key ]['merge_conflict'] = $original_values; |
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.
Maybe merge_conflict should use the source UUID as they key for each value here? Then we'll be able to look up later which snapshot the value came from.
php/class-post-type.php
Outdated
| jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action']"); | ||
| jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action2']"); | ||
| }); | ||
| </script> |
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.
Indentation problem.
php/class-post-type.php
Outdated
| <script type="text/javascript"> | ||
| jQuery(document).ready(function() { | ||
| jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action']"); | ||
| jQuery('<option>').val('merge_snapshot').text('<?php esc_html_e( 'Merge Snapshot', 'customize-snapshots' ); ?>').appendTo("select[name='action2']"); |
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.
These lines aren't right. There could be a translation string that has an apostrophe and this would cause a JS error. I suggest refactoring as follows:
jQuery( function( $ ) {
var optionText = <?php echo wp_json_encode( __( 'Merge Snapshot', 'customize-snapshots' ) ); ?>;
$( 'select[name="action"], select[name="action2"]' ).each( function() {
var option = $( '<option>', {
text: optionText,
value: 'merge_snapshot'
} );
$( this ).append( option );
} );
} );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.
Super clean 👍 🙇
|
@PatelUtkarsh is this still a WIP or is it complete from your perspective? |
|
@westonruter Resolve Conflict UI part is remaining. |
|
@westonruter I am working on Resolve Conflict UI on a separate branch so this can be reviewed and merged if you think Resolve Conflict UI is not critical. |
Fixes #67