-
Notifications
You must be signed in to change notification settings - Fork 804
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
Blocks: introduce new Podcast Episodes block #14928
Conversation
@marekhrabe I'll let you take that one from there! |
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: April 7, 2020. |
Adding the "Needs Product Review" as a reminder that it'd be great if you could publish about this new block, its target deadline, and its deployment strategy as explained here: Thank you! |
I've probably done too much here already, but it works on the front-end, building on the RSS block and re-using Core's audio playlist scripts. The editor preview needs attention. It's very much possible this will not be the route we'll end up taking, so let me know what you think, or rip it out and start over :) |
@@ -0,0 +1,12 @@ | |||
const urlValidator = url => ! url || url.startsWith( 'http' ); |
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.
You could look at isUrl()
from @wordpress/url
: https://github.com/WordPress/gutenberg/blob/03e3c05eca3ee66b99655e79d2939dafe27a46e3/packages/url/src/is-url.js#L16 or using new URL()
native JS API.
$rss = fetch_feed( $attributes['url'] ); | ||
|
||
if ( is_wp_error( $rss ) ) { | ||
return '<div class="components-placeholder"><div class="notice notice-error"><strong>' . __( 'RSS Error:', 'jetpack' ) . '</strong> ' . $rss->get_error_message() . '</div></div>'; |
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.
Remember to HTML escape the error.
84b296b
to
8862522
Compare
I've rebased this branch and removed extra commits to be picked up in follow-up PRs as described in this comment (see also p1583833360124800-slack-ajax). For the removed stuff, I created two separate branches with it:
I've tested this one locally and it looks good to me, as far as the scaffolding task is concerned:
|
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 tests are currently failing because the scaffolding introduces variables you can use to build your block, but haven't yet. I'd recommend removing them or adding comments so the tests can pass. You can add those back later on as you build your PRs.
6:23 error 'withNotices' is defined but never used no-unused-vars
16:2 error 'attributes' is defined but never used no-unused-vars
20:2 error 'setAttributes' is defined but never used no-unused-vars
27:10 error 'notice' is assigned a value but never used no-unused-vars
27:18 error 'setNotice' is assigned a value but never used no-unused-vars
30:8 error 'setErrorNotice' is assigned a value but never used no-unused-vars
Caution: This PR has changes that must be merged to WordPress.com |
9f20ab5
to
1b6ba78
Compare
@Automattic/jetpack-crew When you get a chance, I'd love to get this in early so we can start to rebase and continue with follow up PRs. Thank you! |
I'll take a look in a bit. To speed things up, this may help:
For more info about this: p1583857050271600-slack-ajax |
Howdy! The Jetpack team has disappeared for a few days to a secret island lair to concoct new ways to make Jetpack one hundred billion percent better. As a result, your Pull Request may not be reviewed right away. Do not worry, we will be back next week to look at your work! Thank you for your understanding. |
No worries about this ^ This is Hack week for us, but I'll take a look at the PR anyway. |
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.
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 should be good to ship as is, so y'all can start iterating on this.
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 code looks good, keeping in mind that it's just a scaffold for the real thing. Let's merge!
* Initial changelog entry * Changelog: add #14904 * Changelog: add #14910 * Changelog: add #14913 * Changelog: add #14916 * Changelog: add #14922 * Changelog: add #14924 * Changelog: add #14925 * Changelog: add #14928 * Changelog: add #14840 * Changelog: add #14841 * Changelog: add #14842 * Changelog: add #14826 * Changelog: add #14835 * Changelog: add #14859 * Changelog: add #14884 * Changelog: add #14888 * Changelog: add #14817 * Changelog: add #14814 * Changelog: add #14819 * Changelog;: add #14797 * Changelog: add #14798 * Changelog: add #14802 * Changelog: add #13676 * Changelog: add #13744 * Changelog: add #13777 * Changelog: add #14446 * Changelog: add #14739 * Changelog: add #14770 * Changelog: add #14784 * Changelog: add #14897 * Changelog: add #14898 * Changelog: add #14968 * Changelog: add #14985 * Changelog: add #15044 * Changelog: add #15052 * Update to remove Podcast since it remains in Beta * Changelog: add #14803 * Changelog: add #15028 * Changelog: add #15065 * Changelog:add #14886 * Changelog: add #15118 * Changelog: add #14990 * Changelog: add #14528 * Changelog: add #15120 * Changelog: add #15126 * Changelog: add #15049 * Chanegelog: add #14852 * Changelog: add #15090 * Changelog: add #15138 * Changelog: add #15124 * Changelog:add #15055 * Changelog: add #15017 * Changelog: add #15109 * Changelog: add #15145 * Changelog:add #15096 * Changelog:add #15153 * Changelog: add #15133 * Changelog: add #14960 * Changelog: add #15127 * Changelog: add #15056 * Copy current changelog to changelog archive. * Clarify changelog description
Changes proposed in this Pull Request:
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
Proposed changelog entry for your changes: