-
Notifications
You must be signed in to change notification settings - Fork 2k
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
SDK: Add Jetpack publicize extension #26389
Conversation
const { | ||
withSelect, | ||
withDispatch, | ||
} = window.wp.data; |
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 shouldn't reference window
here - = wp.data
or better @wordpress/data
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.
Fixed in bc312fd
window.clearInterval( popupTimer ); | ||
refreshCallback(); | ||
} | ||
}, 500 ); |
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 would be favorable not to use the setInterval()
here - I'm curious why it's here. in general I think it's nice to rely on active propagation of the event rather than passive as is done here. that is, we should make a decision to check again in the future instead of relying on the system to automatically keep it going (that way we can prevent accidental background tasks from forming)
also, why was 500 chosen as the timeout value? (I'm starting to wonder if this was copy/pasted from Stack-Overflow without citation). There's a very similar solution there that uses 1000
@tyxla I think it's fine to merge this code here from where it came from and recognize that it needs work. we can iterate further from here. |
Yeah, totally - this applies for the rest of the blocks we already merged, too 😉 |
bc312fd
to
94ba41a
Compare
This PR adds the Jetpack Publicize extension to the set of Gutenberg extensions, based on the previous work done by @c-shultz in: Automattic/jetpack#9934.
The code hasn't been edited yet - the idea is to be able to build it in Calypso and use it directly in Jetpack as a Gutenberg plugin. The only critical fix was the import of
PluginPrePublishPanel
, which is now in the publicedit-post
API (see WordPress/gutenberg#6798).To test:
client/gutenberg/extensions/publicize/build/publicize-editor.js
to Jetpack's folder -modules/publicize/assets/block.js
(don't mind the file name we're using for now)try/publicize-gutenberg-block-externally-built
branch in your Jetpack installation (see Demo: Try externally built Publicize extension for Gutenberg jetpack#9963).Fixes #26362.