-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add Gutenberg prepublish panel to Autotweet post #44
Conversation
package.json
Outdated
@@ -4,7 +4,7 @@ | |||
"description": "Automatically tweets a post title, URL, and optional description.", | |||
"scripts": { | |||
"watch": "webpack -wd", | |||
"build": "webpack -p", | |||
"build": "webpack src/js/externals/api-fetch.js -o dist/api-fetch.js -p --module-bind js=babel-loader && webpack -p --config webpack.gutenberg.config.js", |
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.
Building the external api-fetch
script via webpack CLI.
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.
can we add a separate build:externals command, and keep the main build simpler? We would ideally store this in a more persistent externals output location and only rebuild when a new version of some external package is released.
src/js/index.js
Outdated
const AutotweetPrePublishPanelPlugin = () => { | ||
const [ enabledText, setEnabledText ] = useState( '' ); | ||
|
||
useEffect( () => { |
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 we need to use class components for the earlier 5.* WP versions that don't have React with hooks?
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.
Right, we do need to make this work with WP 5.0 & 5.1 for now, which means classes for state.
@johnwatkins0 Thank you for the work here! A few notes:
Would changing the status to draft then re-publishing give you that opportunity?
Outside the scope of this issue
Yes, this makes more sense. Thanks. I am going to do more testing with this. Some initial feedback:
|
export const STORE = '10up/autotweet'; | ||
|
||
export function createAutotweetStore() { | ||
const store = registerStore( STORE, { reducer, actions, selectors } ); |
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!
$failed_filter = function( $data, $id, $key ) use ( $post ) { | ||
if ( intval( $post ) === intval( $id ) && $key === TWITTER_STATUS_KEY ) { | ||
return [ | ||
'status' => 'error', |
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.
nitpick: extra space here before ->
$unknown_filter = function( $data, $id, $key ) use ( $post ) { | ||
if ( intval( $post ) === intval( $id ) && $key === TWITTER_STATUS_KEY ) { | ||
return [ | ||
'status' => 'unknown', |
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.
space
$other_filter = function( $data, $id, $key ) use ( $post ) { | ||
if ( intval( $post ) === intval( $id ) && $key === TWITTER_STATUS_KEY ) { | ||
return [ | ||
'status' => 'other', |
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.
space
@@ -0,0 +1,112 @@ | |||
<?php | |||
/** | |||
* Tests functions admin/post-meta.php. |
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, thanks for adding these tests.
Another good follow up issue: might be as simple as adding a Gutenberg section in the Readme. |
src/js/externals/api-fetch.js
Outdated
@@ -1,3 +1,3 @@ | |||
import apiFetch from '@wordpress/api-fetch'; | |||
|
|||
wp.apiFetch = apiFetch; | |||
global.wp.apiFetch = apiFetch; |
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.
why is global
needed 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.
It's not. I left it there by accident. Removing it.
filename: '[name].js', | ||
path: path.resolve( './dist' ), | ||
}, | ||
externals: { |
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.
Excellent!
@johnwatkins0 Excellent! This is looking really good. Reusing the I left a few small comments/questions and am going to test this locally then will approve. |
src/js/AutotweetPrePublishPanel.js
Outdated
label={ | ||
<span className="autotweet-prepublish__message-label"> | ||
<span>{ __( 'Custom message:', 'autotweet' ) }</span> | ||
<span id="tenup-auto-tweet-counter-wrap" className={ overrideLengthClass() }>{ tweetText.length }</span> |
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 space is collapsing between the text and the count. (https://cl.ly/4f04884a8b03) fix by keeping closing span and opening span on same line with space (</span> <span>...
)
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 struggled with this a bit trying to make this suggestion work, and ended up using
inside the first span.
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.
that works! React loves to collapse whitespace, it usually preserves it if on the same line.
assets/css/admin-auto_tweet.css
Outdated
@@ -52,6 +94,11 @@ span#tenup-auto-tweet-counter-wrap{ | |||
background: rgba(0, 0, 0, 0.07); | |||
font-family: Consolas, Monaco, monospace; | |||
} | |||
|
|||
span.near-limit{ | |||
color: orange; |
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.
improve the contrast, orange was hard to see
color: orange; | |
color: darkorange; |
src/js/AutotweetPrePublishPanel.js
Outdated
// and save data when they update. | ||
this.state = { autotweetEnabled: null, tweetText: null }; | ||
|
||
this.saveData = debounce( this.saveData.bind( this ), 1000 ); |
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.
Let's lower the debounce to 250. I was able to click the tweet this checkbox then the publish button in under a second and the post didn't publish :) https://cl.ly/6ac0e9399675
this.saveData = debounce( this.saveData.bind( this ), 1000 ); | |
this.saveData = debounce( this.saveData.bind( this ), 250 ); |
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.
Excellent work here @johnwatkins0 - this is looking really good and worked well in my testing. I left a few small core change requests, once these are in I will approve for merge!
@adamsilverstein Thanks @adamsilverstein . Pushed a few very tiny commits based on your most recent round of feedback. Let me know if you see anything else. |
I'll look at those conflicts. |
@adamsilverstein I've fixed the conflicts following the merging of #42 |
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.
Fantastic!
This is intended to resolve #13.
It depends on #37.Description of the Change
This adds a PrePublishPanel to the Gutenberg editor when the user is working on a new post in a post type supporting autotweet.
While publishing, the user sees:
After publication, we also add a PostStatusInfo message resembling the message that shown in the pre-Gutenberg implementation:
Alternate Designs
Although #13 mentions
PluginPostPublishPanel
, I instead usedPlugin*Pre*PublishPanel
because the pre-publish panel allows us to re-use most of the existing backend code. The existing backend code sends the Tweet on post save; for a post-publish panel, we might have to create a whole different REST endpoint to handle a separate request to publish the post.I also subjectively feel the prepublish panel is a better UX because it allows the user to think about the tweet along with the post content rather than as an afterthought. If there is a good reason to use
PluginPostPublishPanel
, however, the work in this PR would not need to be adjusted very much.Benefits
Pre-5.0 functionality provided by this plugin is now provided via the Gutenberg editor.
Possible Drawbacks
A few things have come up that I believe should be added as to-dos for the next release:
Verification Process
Checklist:
Changelog Entry
Adds Autotweet functionality to the Gutenberg editor.