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

Show notification when a post a restored from trash #347

Merged
merged 4 commits into from Mar 3, 2017

Conversation

Projects
None yet
2 participants
@sayedtaqui
Copy link
Collaborator

commented Feb 27, 2017

Fixes #217

@sayedtaqui

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 27, 2017

Ready for review.

@@ -846,6 +846,7 @@ public function enqueue_scripts() {
__( 'Customize Object Selector', 'customize-posts' )
)
),
'trashPostNotification' => __( 'This has been untrashed. If you publish changes now, this post will be published. Move back to trash to avoid this.' , 'customize-posts' ),

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2017

Contributor

It may not actually be published. If the post was a draft then it will be restored as a draft, right?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Mar 2, 2017

Author Collaborator

Correct, how about changing it to this?
"This has been untrashed. If you publish changes now, its status will change to the selected status. Move back to trash to avoid this."

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 2, 2017

Contributor

Yes, that's good.

@@ -32,6 +34,10 @@
panel.deferred.embedded.done(function() {
panel.setupPanelActions();
});

api.bind( 'saved', function() {

This comment has been minimized.

Copy link
@westonruter

westonruter Feb 27, 2017

Contributor

This will trigger even when updating a changeset draft via Customize Snapshots. Is that intended? If you want it to only run when the changeset is published as a whole, then I think you need to make sure that the changeset status is publish, like:

api.bind( 'saved', function( data ) {
    if ( 'publish' === data.changeset_status ) {
        panel.removeTrashNotifications();
    }
} );

This is adapted from https://github.com/xwp/wp-customize-featured-content-demo/blob/8bf4d6531adb029fbaf1e4bebb967e57989d5edd/js/featured-items-panel.js#L148-L153

Humm. But actually this code is also being used for purging the trashed items as well:

// Purge trashed posts and update client settings with saved values from server.
api.bind( 'saved', function( data ) {
if ( data.saved_post_setting_values ) {
component.updateSettingsQuietly( data.saved_post_setting_values );
}
component.purgeTrash();
} );

So maybe that other location in Customize Posts needs to be updated as well.

Or maybe this is all intended, to purge the trashed items upon saving an explicit draft as well as upon publishing, and if so you can ignore this comment 😄

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Mar 2, 2017

Author Collaborator

It was intended because if snapshot saves it as draft or any other status, it would eventually get published when they publish the snapshot, so the user should know this right?

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 2, 2017

Contributor

I think the way you have it is a better experience. If I hit Move to Trash, and then hit Save Draft, I pretty clearly don't want the trashed item around anymore and I'm unlikely to be wanting to undo it. So removing the trashed post sections upon explicit Save makes sense to me.

@westonruter westonruter added this to the Next Minor Release milestone Mar 3, 2017

} );

statusControl.setting.bind( function() {
statusControl.notifications.remove( panel.trashNotificationCode );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 3, 2017

Contributor

So this will remove the notification whenever a user makes a change to the setting? Probably should call statusControl.setting.unbind() to remove this function handler after it runs once, right?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Mar 3, 2017

Author Collaborator

Yes, good point

message: api.Posts.data.l10n.trashPostNotification
} );

statusControl = api.control( postData.section.id + '[post_status]' );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 3, 2017

Contributor

It's unlikely, but this should probably check to see if statusControl exists (like you're doing in removeTrashNotifications below), and if not then short circuit.

statusControl = api.control( postData.section.id + '[post_status]' );
statusControl.deferred.embedded.done( function() {
statusControl.notifications.add( panel.trashNotificationCode, notification );
} );

This comment has been minimized.

Copy link
@westonruter

westonruter Mar 3, 2017

Contributor

Just curious why the statusControl.deferred.embedded is needed here. The statusControl.notifications collection should exist from the moment that a control is initialized, so I don't think that you have to wait for embedding?

This comment has been minimized.

Copy link
@sayedtaqui

sayedtaqui Mar 3, 2017

Author Collaborator

I wasn't sure, but right embedded is not required here.

@westonruter westonruter merged commit 621f875 into develop Mar 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the enhancement/issue-217 branch Mar 3, 2017

@westonruter westonruter modified the milestones: Next Minor Release, 0.8.6 Jun 7, 2017

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.