Skip to content
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

Podcast Player: Increase podcast shown #21661

Merged
merged 14 commits into from
Nov 17, 2021

Conversation

amustaque97
Copy link
Contributor

@amustaque97 amustaque97 commented Nov 5, 2021

Fixes #21291

Changes proposed in this Pull Request:

  • Created a new API to retrieve number of tracks.
  • Added a method to call the above mentioned api
  • Updating the max value of rangeControlUI

Jetpack product discussion NO

Does this pull request change what data or activity we track or use? NO

Testing instructions:

function my_jetpack_podcast_helper_tracks_quantity( $tracks_quantity ) {
	return 42;
}
add_filter( 'jetpack_podcast_helper_tracks_quantity', 'my_jetpack_podcast_helper_tracks_quantity' );
  • Reload the page and check number of items button in podcast settings UI.
  • Change value instead return 15; of filter to some different value and it will update max limit in the rangeControlUI as well.

Screenshots

  • Number of Items

image

cc @samiff , I have created a separate branch because there are a lot of changes in the previous branch.

This commit is a part of issue Automattic#21291. This commit is pushed in a
separate branch to avoid confusion from the previous suggested solution.
In this PR changes are:
* Introduced a new endpoint `track-quantity` to return number of tracks.
* Added a new method in the front end codebase to call the above mentioned API.
* After calling update `defaultMaxItems` of the rangeControlUI
@github-actions github-actions bot added [Block] Podcast Player [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Nov 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.



Jetpack plugin:

  • Next scheduled release: December 7, 2021.
  • Scheduled code freeze: November 30, 2021.

@github-actions github-actions bot added the OSS Citizen This Pull Request was opened by an Open Source contributor. label Nov 5, 2021
@amustaque97 amustaque97 changed the title Add/increase podcast shown Podcast Player: Increase podcast shown Nov 5, 2021
@samiff samiff self-requested a review November 8, 2021 20:49
@samiff samiff added the [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it label Nov 8, 2021
@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 16, 2021
In this commit, we're deprecating `jetpack_podcast_helper_list_quantity` filter and introducing
a new filter `jetpack_podcast_helper_tracks_quantity`. New filter will work without $rss.
Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good and is working to do what we want 👍 Left some suggestions and a comment in hopes of polishing things up a bit.

amustaque97 and others added 5 commits November 17, 2021 16:41
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
In this commit, I have removed url parameter from fetchTrackQuantity api method and
I have written a separate hook to call the same function.
@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 17, 2021
@jeherve jeherve added this to the jetpack/10.4 milestone Nov 17, 2021
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 17, 2021
@samiff samiff self-requested a review November 17, 2021 19:23
Copy link
Contributor

@samiff samiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding those changes, this is looking good! I added two minor suggestions, and please merge the master branch once more into your branch so the checks are able to pass.

I was checking the docs about add_filter but couldn't find what is the best practice to write add_filter I mean whether we should write it in the class __construct or any class method and so on.

Hmm not sure I fully understood the question being asked here so feel free to clarify if I missed something. As an end user, I would be able to do something like

function my_jetpack_podcast_helper_tracks_quantity( $tracks_quantity ) {
	return 42;
}
add_filter( 'jetpack_podcast_helper_tracks_quantity', 'my_jetpack_podcast_helper_tracks_quantity' );

My personal preference would be to add that to a mu-plugin file, or perhaps a child theme's functions.php file. Adding that example snippet seems to work well:

Markup on 2021-11-17 at 12:47:24

amustaque97 and others added 3 commits November 18, 2021 01:35
Co-authored-by: Samiff <samiff@users.noreply.github.com>
Co-authored-by: Samiff <samiff@users.noreply.github.com>
@amustaque97
Copy link
Contributor Author

amustaque97 commented Nov 17, 2021

PR is up to date 🎉

Oh God! I was writing add_filter statement in the wp core functions.php! So stupid 😞

Thanks once again Samiff & jeherve. Really appreciate your efforts and patience ❤️

Hopefully, I will keep this in mind that there is functions.php file in theme as well. 😥

@samiff samiff enabled auto-merge (squash) November 17, 2021 21:27
@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 17, 2021
@samiff samiff merged commit ac26853 into Automattic:master Nov 17, 2021
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 17, 2021
@samiff
Copy link
Contributor

samiff commented Nov 17, 2021

Thanks for the contribution @amustaque97 these changes are now merged and will be shipped as part of Jetpack version 10.4 on 2021-12-07 🎉

@samiff
Copy link
Contributor

samiff commented Nov 17, 2021

Internal: r235233-wpcom , D70255-code

davidlonjon added a commit that referenced this pull request Nov 18, 2021
* master: (28 commits)
  Prepare for 10.4-a.7 (#21797)
  Check if maxwidth exists before trying to call .length (#21785)
  Podcast Player: Increase podcast shown (#21661)
  licensing: jetpack-js-test-runner is a dev dependency (#21794)
  Update dependency @mdn/browser-compat-data to v4.0.11 (#21792)
  Tests: Fix unit tests for wpcom (#21649)
  SSO: Fix Button Text Alignment on Mobile   (#18770)
  Add/my jetpack skeleton (#21464)
  Identity Crisis: add unit tests for has_identity_crisis and get_mismatched_urls (#21754)
  Remove special cases for calling changelogger on packages/changelogger (#21783)
  eslint-config-target-es: Release 1.0.0! (#21766)
  Revert "WPCOM no longer forces home_urls to be http" (#21769)
  WPCOM no longer forces home_urls to be http (#21747)
  Revert and fix "Revert "Assets: do not use the new method yet (#21760)"" (#21763)
  eslint-config-target-es: Fix README.md (#21743)
  codesniffer: Disable CI on PHP 8.1 (#21742)
  RNA Pricing card component: Fix case where prices before and after match (#21757)
  Janitorial: update Jetpack version for 10.4-a.6 cycle (#21762)
  Releases: prepare changelog for 10.4-a.5 release (#21758)
  Assets: do not use the new method yet (#21760)
  ...
@amustaque97 amustaque97 deleted the add/increase-podcast-shown branch November 18, 2021 15:03
davidlonjon added a commit that referenced this pull request Nov 19, 2021
* master: (28 commits)
  Prepare for 10.4-a.7 (#21797)
  Check if maxwidth exists before trying to call .length (#21785)
  Podcast Player: Increase podcast shown (#21661)
  licensing: jetpack-js-test-runner is a dev dependency (#21794)
  Update dependency @mdn/browser-compat-data to v4.0.11 (#21792)
  Tests: Fix unit tests for wpcom (#21649)
  SSO: Fix Button Text Alignment on Mobile   (#18770)
  Add/my jetpack skeleton (#21464)
  Identity Crisis: add unit tests for has_identity_crisis and get_mismatched_urls (#21754)
  Remove special cases for calling changelogger on packages/changelogger (#21783)
  eslint-config-target-es: Release 1.0.0! (#21766)
  Revert "WPCOM no longer forces home_urls to be http" (#21769)
  WPCOM no longer forces home_urls to be http (#21747)
  Revert and fix "Revert "Assets: do not use the new method yet (#21760)"" (#21763)
  eslint-config-target-es: Fix README.md (#21743)
  codesniffer: Disable CI on PHP 8.1 (#21742)
  RNA Pricing card component: Fix case where prices before and after match (#21757)
  Janitorial: update Jetpack version for 10.4-a.6 cycle (#21762)
  Releases: prepare changelog for 10.4-a.5 release (#21758)
  Assets: do not use the new method yet (#21760)
  ...
@inaikem
Copy link

inaikem commented Aug 11, 2022

Edit: I posted this request in the original issue here:
#21291

@github-actions
Copy link
Contributor

Support References

This comment is automatically generated. Please do not edit it.

  • 5469709-zen

@matticbot matticbot added the Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Podcast Player Customer Report Issues or PRs that were reported via Happiness. Previously known as "Happiness Request". OSS Citizen This Pull Request was opened by an Open Source contributor. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Podcast Player: Increase the number of episodes shown
5 participants