Feature/issue 229 191 player enhancements#272
Conversation
# Conflicts: # assets/js/blocks.js # simple-podcasting.php # webpack.config.js
|
@barneyjeffries thanks for the PR! Could you please fill out the PR template with description, changelog, and credits information so that we can properly review and merge this? |
faisal-alvi
left a comment
There was a problem hiding this comment.
@barneyjeffries thank you for working on this. The player enhancement looks good.
BE:
FE:
However, the following checks are failing, can you please look into this?
| ); | ||
|
|
||
| const { getCurrentPost } = select('core/editor'); | ||
| const postDetails = getCurrentPost(); |
There was a problem hiding this comment.
It would be good to subscribe to the post data so the block is updated when the post title is edited https://gutenberg.10up.com/guides/data-api/#examples
There was a problem hiding this comment.
@peterwilsoncc Could you please add some example snippet here? I was trying to use data-api example from above link but getting error called Invalid hook call since it's a class component. Tried in componentDidMount lifecycle hook as well got same error. Thanks in advance :)
There was a problem hiding this comment.
I think you'll need to do it for the post title and show ID separately. Here's an example for the title:
// Top of file
const { select, subscribe } = wp.data;
// Around here for the post title
const { getCurrentPost, getEditedPostAttribute } = select('core/editor');
let postTitle = getEditedPostAttribute( 'title' );
subscribe( () => {
const newPostTitle = getEditedPostAttribute( 'title' );
if ( postTitle !== newPostTitle ) {
postTitle = newPostTitle;
}
});
// Below
{ postTitle } // instead of { postDetails.title }From the notes in the block editor handbook, you'll want to use functions that refer to the edited data rather than the current data.
| 'Explicit: ', | ||
| 'simple-podcasting' | ||
| )} | ||
| {explicit} |
There was a problem hiding this comment.
I noticed this defaults to a blank value unless the user modifies the dropdown, it needs to default to the attributes default value
There was a problem hiding this comment.
I'm still seeing this.
peterwilsoncc
left a comment
There was a problem hiding this comment.
I've added a few note and provided the requested code example.
| ); | ||
|
|
||
| const { getCurrentPost } = select('core/editor'); | ||
| const postDetails = getCurrentPost(); |
There was a problem hiding this comment.
I think you'll need to do it for the post title and show ID separately. Here's an example for the title:
// Top of file
const { select, subscribe } = wp.data;
// Around here for the post title
const { getCurrentPost, getEditedPostAttribute } = select('core/editor');
let postTitle = getEditedPostAttribute( 'title' );
subscribe( () => {
const newPostTitle = getEditedPostAttribute( 'title' );
if ( postTitle !== newPostTitle ) {
postTitle = newPostTitle;
}
});
// Below
{ postTitle } // instead of { postDetails.title }From the notes in the block editor handbook, you'll want to use functions that refer to the edited data rather than the current data.
|
|
||
| <div className="wp-block-podcasting-podcast__details"> | ||
|
|
||
| {displayEpisodeNumber && episodeNumber && ( |
There was a problem hiding this comment.
This causes either no/turning off the episode number to display to also hide the title. I think what you want here is a check for the episode title display.
The eposide number check should be part of line 463
| 'Explicit: ', | ||
| 'simple-podcasting' | ||
| )} | ||
| {explicit} |
There was a problem hiding this comment.
I'm still seeing this.
|
@Firestorm980 Could you please resolve the conflicts? Thanks! |
|
@gusaus do you want to give this a test to see how it aligns to what you had in mind and whether there are any tweaks/iterations you'd recommend? |
|
@jeffpaul Thanks for keeping me looped in! Good timing as I was just circling back to this feature request https://github.com/OpenProducer/newspack-radio-pro/issues/3 for the Newspack radio branch we maintain (https://github.com/OpenProducer/newspack-platform/tree/radio). With regards to testing, is there a particular branch we should evaluate? I tried installing the following (to a clean install of Newspack and core) and got the same message when I tried to activate: As a non-coder (and still relatively new to WordPress), I could well be testing the wrong thing (and the wrong way). That said, I'm very interested in testing, providing feedback, and potentially including in our Newspack platform! |
|
@gusaus youll want to use the Zip action in the repo to build a plugin zip file from this PRs branch to get a good install option for testing. |
|
@gusaus you can use this ZIP file for testing: simple-podcasting.zip |
|
@jeffpaul Thank you so much for providing the ZIP! That said, I actually researched the Zip action workflow (thanks to my friend ChatGPT) and managed to create a zip on my fork yesterday. Comparing them today, it looks like they're almost identical. I'm amazed! With regards to testing, I've installed on a Newspack sandbox and created a few podcasts and episodes (using the Podcast block) and it seems like all the enhancements outlined in this PR are working! Would love to see some of the other enhancements you listed in #191 go in.
Considering so many publishers are podcasting, I'm wondering if there's an opportunity to work with Newspack/Automattic on integrating and including in Newspack. |
Thanks for testing @gusaus! After discussing internally, we're going to pull these three items out to separate issues so we can proceed with getting this PR finalized and merged. |
|
@dkotter Great to know and really appreciate all the work y'all are doing! We've set up new branch on our Newspack platform (https://github.com/OpenProducer/newspack-platform/tree/podcast) and will continue to test provide feedback in the corresponding issues. |






Description of the Change
New features added to the player.
Closes #191 and #229
Alternate Designs
Possible Drawbacks
Verification Process
Checklist:
Changelog Entry
Credits
Props @barneyjeffries @Firestorm980 @mehidi258 @Sidsector9