-
Notifications
You must be signed in to change notification settings - Fork 30
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
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? |
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.
@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?
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.
assets/js/edit.js
Outdated
@@ -144,6 +156,19 @@ class Edit extends Component { | |||
</BlockControls> | |||
); | |||
|
|||
const { getCurrentPost } = select('core/editor'); | |||
const postDetails = getCurrentPost(); |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
assets/js/edit.js
Outdated
'Explicit: ', | ||
'simple-podcasting' | ||
)} | ||
{explicit} |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing 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.
I've added a few note and provided the requested code example.
assets/js/edit.js
Outdated
@@ -144,6 +156,19 @@ class Edit extends Component { | |||
</BlockControls> | |||
); | |||
|
|||
const { getCurrentPost } = select('core/editor'); | |||
const postDetails = getCurrentPost(); |
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 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.
assets/js/edit.js
Outdated
|
||
<div className="wp-block-podcasting-podcast__details"> | ||
|
||
{displayEpisodeNumber && episodeNumber && ( |
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 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
assets/js/edit.js
Outdated
'Explicit: ', | ||
'simple-podcasting' | ||
)} | ||
{explicit} |
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'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 OpenProducer/newspack-platform#22 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