Skip to content
This repository was archived by the owner on Dec 27, 2022. It is now read-only.

Add snapshot fork feature #90

Merged
westonruter merged 41 commits into from
Dec 13, 2017
Merged

Add snapshot fork feature #90

westonruter merged 41 commits into from
Dec 13, 2017

Conversation

@PatelUtkarsh
Copy link
Member

@PatelUtkarsh PatelUtkarsh commented Sep 5, 2016 •
edited by postphotos
Loading

Fixes: #41
Any UI improvements are welcome.

Remaining:

  • Post meta Meta fork/copy
  • Should append (fork) in a post title or not confirm and update.
  • Unit-test

@coveralls
Copy link

coveralls commented Sep 5, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-3.7%) to 87.314% when pulling 38d9e60 on feature/snapshot-fork into 010daf7 on develop.

@coveralls
Copy link

coveralls commented Sep 5, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-4.2%) to 86.783% when pulling d1dc207 on feature/snapshot-fork into 010daf7 on develop.

@coveralls
Copy link

coveralls commented Sep 6, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-4.5%) to 86.517% when pulling 8d26c58 on feature/snapshot-fork into 010daf7 on develop.

@PatelUtkarsh PatelUtkarsh mentioned this pull request Sep 6, 2016
Closed
@coveralls
Copy link

coveralls commented Sep 6, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.8%) to 90.147% when pulling f3aadad on feature/snapshot-fork into 010daf7 on develop.

@coveralls
Copy link

coveralls commented Sep 7, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 90.32% when pulling a0d7971 on feature/snapshot-fork into 010daf7 on develop.

@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.6%) to 90.345% when pulling 0c20ece on feature/snapshot-fork into 010daf7 on develop.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 12, 2016

Coverage Status

Coverage decreased (-0.6%) to 90.345% when pulling 0c20ece on feature/snapshot-fork into 010daf7 on develop.

@PatelUtkarsh PatelUtkarsh changed the title [WIP] Add snapshot fork feature Add snapshot fork feature Sep 16, 2016
@PatelUtkarsh
Copy link
Member Author

PatelUtkarsh commented Sep 16, 2016

@westonruter / @valendesigns This PR is ready for review.
This will conflict with #84 and I will be happy to resolve conflicts.

valendesigns
valendesigns reviewed Sep 16, 2016
View reviewed changes
js/customize-snapshots-admin.js Outdated

component.forkClick = function() {
var $forkButton = $( '#snapshot-fork' ),
$forkSnipper = $( '.snapshot-fork-spinner' ),
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change this to $spinner or at the very least fix the typo and change it to $forkSpinner.

js/customize-snapshots-admin.js Outdated
var $forkButton = $( '#snapshot-fork' ),
$forkSnipper = $( '.snapshot-fork-spinner' ),
$forkList = $( '#snapshot-fork-list' ),
forkItemTemplate = wp.template( 'snapshot-fork-item' );
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return space.

js/customize-snapshots-admin.js Outdated
request = wp.ajax.post( 'snapshot_fork', {
ID: $forkButton.data( 'post-id' ),
nonce: $forkButton.data( 'nonce' )
} );
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return space.

js/customize-snapshots-admin.js Outdated
} );
request.always( function() {
$forkSnipper.removeClass( 'is-active' );
} );
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return space.

js/customize-snapshots-admin.js Outdated
var item;
item = $( $.trim( forkItemTemplate( data ) ) );
$forkList.append( item );
} );
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return space.

php/class-customize-snapshot-manager.php Outdated
* @param String $hook Current page.
*/
public function enqueue_admin_scripts( $hook ) {
global $post;
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be moved into the conditional, and add return space after the global.

php/class-post-type.php
$frontend_view_url = get_permalink( $post->ID );
echo sprintf(
'<a href="%s" class="button button-secondary">%s</a>',
'<a href="%s" class="button button-secondary">%s</a> ',
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the extra whitespace in this line change a typo?

Copy link
Member Author

@PatelUtkarsh PatelUtkarsh Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a change.

Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's just to add visual space to the buttons then it should probably be done with CSS on #snapshot-fork instead.

Copy link
Member Author

@PatelUtkarsh PatelUtkarsh Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just being consistent with

wp-customize-snapshots/php/class-post-type.php

Line 349 in 0c20ece

'<a href="%s" class="button button-secondary">%s</a> ',

Should I remove space between both button and add CSS?

php/class-post-type.php Outdated
echo sprintf( '%1$s %2$s %3$s', esc_html__( 'Modified:', 'customize-snapshots' ), esc_html( get_the_modified_date( '' ) ), esc_html( get_the_modified_time( '' ) ) ) . '<br>';
echo '</p>';

$fork_markup = '<a href="#" id="snapshot-fork" class="button button-secondary" data-post-id="' . $post->ID . '" data-nonce="' . wp_create_nonce( 'snapshot-fork' ) . '">' . esc_html__( 'Fork', 'customize-snapshots' ) . '</a>';
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a button not a link and also using sprintf would clean things up. Something like:

$fork_markup = sprintf(
    '<button id="snapshot-fork" class="button button-secondary" data-post-id="%s" data-nonce="%s">%s</button><span class="spinner snapshot-fork-spinner"></span>',
    $post->ID,
    wp_create_nonce( 'snapshot-fork' ),
    esc_html__( 'Fork', 'customize-snapshots' )
);

php/class-post-type.php
} ?>
</ul><?php
if ( $post->post_parent ) {
$parent = get_post( $post->post_parent );
Copy link
Contributor

@valendesigns valendesigns Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could probably remove the &nbsp; and just add a space.

@coveralls
Copy link

coveralls commented Sep 16, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 90.345% when pulling c5ce63e on feature/snapshot-fork into 010daf7 on develop.

@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.6%) to 90.22% when pulling fae0e09 on feature/snapshot-fork into 59b8b66 on develop.

@coveralls
Copy link

coveralls commented Oct 12, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.3%) to 90.554% when pulling a9f920d on feature/snapshot-fork into 59b8b66 on develop.

@PatelUtkarsh
Copy link
Member Author

PatelUtkarsh commented Nov 29, 2017 •
edited
Loading

Todo:

  • Test
  • Style @sayedtaqui

@coveralls
Copy link

coveralls commented Dec 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 80.786% when pulling 52f14c9 on feature/snapshot-fork into 91b0827 on develop.

@coveralls
Copy link

coveralls commented Dec 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 80.786% when pulling c60708f on feature/snapshot-fork into 91b0827 on develop.

@coveralls
Copy link

coveralls commented Dec 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 80.786% when pulling b21d282 on feature/snapshot-fork into 91b0827 on develop.

@coveralls
Copy link

coveralls commented Dec 4, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 80.786% when pulling 384f17a on feature/snapshot-fork into 91b0827 on develop.

@PatelUtkarsh
Copy link
Member Author

PatelUtkarsh commented Dec 5, 2017 •
edited
Loading

Looks good, Ready for review. @westonruter

@PatelUtkarsh PatelUtkarsh removed their assignment Dec 5, 2017
@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.6%) to 80.276% when pulling 2217037 on feature/snapshot-fork into 68e9990 on develop.

2 similar comments
@coveralls
Copy link

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.6%) to 80.276% when pulling 2217037 on feature/snapshot-fork into 68e9990 on develop.

@coveralls
Copy link

coveralls commented Dec 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.6%) to 80.276% when pulling 2217037 on feature/snapshot-fork into 68e9990 on develop.

@westonruter
Focus on newly-added forked snapshot in metabox after clicking fork b…
037384f 
…utton

* Add fork button to Changeset Forks metabox.
* Increase number of forked changesets listed, with showing message if more than 100 exist.
@westonruter
Add missing wp_slash() for insertion of post and postmeta
4ad003a 
Improve jsdoc
@westonruter
Fix covers tag in test_force_at_least_one_revision
f1c1464
@westonruter westonruter force-pushed the feature/snapshot-fork branch 2 times, most recently from 81499a3 to 9506db0 Compare December 13, 2017 04:26
@westonruter
Copy link
Contributor

westonruter commented Dec 13, 2017 •
edited
Loading

It appears that the coveralls Composer package is broken. I can see:

Skipped installation of bin bin/coveralls for package satooshi/php-coveralls: file not found in package

I'll disable it.

@westonruter westonruter force-pushed the feature/snapshot-fork branch from 9506db0 to 49afc85 Compare December 13, 2017 04:36
@westonruter westonruter merged commit e06641b into develop Dec 13, 2017
@westonruter westonruter deleted the feature/snapshot-fork branch December 13, 2017 04:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@valendesigns valendesigns valendesigns left review comments

@westonruter westonruter westonruter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Next Major Release

Development

Successfully merging this pull request may close these issues.

5 participants

@PatelUtkarsh @coveralls @valendesigns @westonruter @mohdsayed