-
Notifications
You must be signed in to change notification settings - Fork 156
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
Toggle Distributor controls when post status changes #1022
Toggle Distributor controls when post status changes #1022
Conversation
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've added a few notes. the main item that needs improvement is to prevent the errors on the new post screen.
When hacking the syndicatable()
function to test this out (by adding return true at the start ;) I noticed that the post title needs to be updated via JavaScript too.
The UI shows the post title on page load rather than the post title at publication. In this screen shot it shows as "Auto Draft" as that's the default title WP uses.
package.json
Outdated
@@ -37,6 +37,7 @@ | |||
}, | |||
"scripts": { | |||
"build": "wp-scripts build", | |||
"watch": "wp-scripts build --watch", |
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.
The start
script on line 54 already does this (wp-scripts start
being an alias).
Did you add this because watch
is a more common name than start
? I've been considering the same because it's a more common 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.
Ah, I didn't realize start
does the same thing. I did a search in package.json
for the word watch
and couldn't find anything, so I added it.
Do you want me to remove this or alias it to npm run start
?
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.
Do you want me to remove this or alias it to
npm run start
?
Let's remove it for now. I'll review our other open-source plugins for the common names, whether it be dev
, watch
or something else.
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 this is really close, just a few things I've noticed.
Instead of using dtGutenberg
to store the title, use the dt
object. That's available wherever the admin bar is: classic editor, block editor and on the front end.
I've pushed 3367955 to include the page title at load in the dt
object, that will ensure it's present for the front end and the classic editor.
I noticed on the classic editor, the menu bar shows and allows a user to distribute a draft.
For the classic editor, I think we'll need to do something to prevent the Distributor menu from displaying on unpublished posts (the classic editor uses a full refresh) so it can be hidden. Maybe check use_block_editor_for_post()
. You'll need to confirm the classic-editor-remember
meta data has been added to the auto-draft at that point.
I'm trying to figure out if we'll need to protect against users unhiding the menu bar in Gutenberg and pushing a draft/non-distributable status to another site. Unfortunately, I haven't been able to figure out how we might go about this.
Thanks for the progress so far, it's looking really nice.
assets/js/gutenberg-plugin.js
Outdated
select( 'core/editor' ).getCurrentPost() | ||
); | ||
// Make the post title available to the top menu. | ||
dtGutenberg.postTitle = post.title; |
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.
dtGutenberg.postTitle = post.title; | |
dt.postTitle = post.title; |
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 is strange. If I modify the const { document, dtGutenberg, MouseEvent } = window;
line and change it to const { document, dtGutenberg, MouseEvent, dt } = window;
, dt
is undefined. However if I don't make that modification I'm able to reference it in code directly via dt.postTitle
or window.dt.postTitle
. Going with window.dt.postTitle
for now as it's more explicit and doesn't require an eslint exception, but please let me know if you have a better suggestion.
assets/js/push.js
Outdated
// If the post title has changed, we need to reload the template. | ||
if ( | ||
distributorPushWrapper.classList.contains( 'loaded' ) && | ||
dtGutenberg.postTitle === dtGutenberg.previousPostTitle |
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.
dtGutenberg.postTitle === dtGutenberg.previousPostTitle | |
dt.postTitle === dt.previousPostTitle |
assets/js/push.js
Outdated
return; | ||
} | ||
dtGutenberg.previousPostTitle = dtGutenberg.postTitle; |
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.
dtGutenberg.previousPostTitle = dtGutenberg.postTitle; | |
dt.previousPostTitle = dt.postTitle; |
assets/js/push.js
Outdated
@@ -394,13 +399,15 @@ jQuery( window ).on( 'load', () => { | |||
connections: mustacheData.connections, | |||
foundConnections: mustacheData.connections.length, | |||
showSearch: 5 < mustacheData.connections.length, | |||
postTitle: dtGutenberg.postTitle, |
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.
postTitle: dtGutenberg.postTitle, | |
postTitle: dt.postTitle, |
assets/js/push.js
Outdated
} | ||
); | ||
|
||
setDisabledConnections(); | ||
} else { | ||
distributorPushWrapper.innerHTML = template( { | ||
connections: dtConnections, | ||
postTitle: dtGutenberg.postTitle, |
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.
postTitle: dtGutenberg.postTitle, | |
postTitle: dt.postTitle, |
Are you referring to the draft created on the source side, or on the target side? I've done some digging and familiarized myself with the Classic Editor plugin and how it uses this metadata, but am still unsure what the goal of this requirement is. |
I'm referring to the source site, so the check you pushed to
It looks like the $asset_file = DT_PLUGIN_PATH . '/dist/js/gutenberg-syndicated-post.min.asset.php';
// Fallback asset data.
$asset_data = array(
'version' => DT_VERSION,
'dependencies' => array(),
);
if ( file_exists( $asset_file ) ) {
$asset_data = require $asset_file;
}
$asset_data['dependencies'][] = 'dt-push'; // NEW LINE.
wp_enqueue_script( 'dt-gutenberg-syndicated-post', plugins_url( '/dist/js/gutenberg-syndicated-post.min.js', __DIR__ ), $asset_data['dependencies'], $asset_data['version'], true ); |
@peterwilsoncc Thanks. PR updated. |
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.
Looks good to me, thanks so much for your patience over the last few days.
I took the liberty of pushing an E2E test in d9f355a to ensure the distributor panel works as expected.
As the block editors selectors change from time-to-time, it may fail in which case I will revert and merge the existing work as is.
…blish." This reverts commit d9f355a.
Description of the Change
Renders the Distributor element in the admin bar whether the post status is distributable or not. If it's not, the element is hidden by CSS. When the post status becomes distributable, the element is displayed.
Also adds a
watch
command to thenpm
scripts.Closes #141
How to test the Change
Switch to draft
and unpublish the post. Note that the admin bar and Distributor Inspector element are as they were in step 1.Changelog Entry
Credits
Props @ggutenberg
Checklist: