Skip to content

Conversation

@aaryansinhaa
Copy link

Added Playlist duration;

  • Used video_channels to store duration of each video in seconds, used an aggregator and added it to the schema of the playlist.
  • Displayed that view in the PlaylistView component and PlaylistPanel component.

I have modified the testcases in the integration_tests and have locally tested. All test cases were passed

Please see the screenshots of the modification in UI for your reference
youtube_zim_ss1
youtube_zim_ss2
youtube_zim_ss3

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Thank you! See inline comments.

Also please do not replace yarn with npm (do not delete yarn.lock and add package-lock.json) ; this needs to be discussed first in a separate issue (and I'm not convinced about the move yet)

And please add an entry to CHANGELOG.md in the "Unreleased" section

@aaryansinhaa
Copy link
Author

aaryansinhaa commented Dec 2, 2025

Hey @benoit74 , really sorry for such a sloppy PR before, I have fixed and refactored my code.
Screenshots of the updated UI:
image
image

@codecov
Copy link

codecov bot commented Dec 3, 2025

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.95%. Comparing base (41d7136) to head (5aef628).

Files with missing lines Patch % Lines
scraper/src/youtube2zim/scraper.py 0.00% 9 Missing ⚠️
scraper/src/youtube2zim/youtube.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##            main    #435      +/-   ##
========================================
- Coverage   0.96%   0.95%   -0.02%     
========================================
  Files         11      11              
  Lines       1136    1148      +12     
  Branches     148     150       +2     
========================================
  Hits          11      11              
- Misses      1125    1137      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aaryansinhaa
Copy link
Author

I believe that the error was because I did not update the cyprus tests for zimui, will fix it asap, sorry for inconvenience.

@benoit74
Copy link
Collaborator

benoit74 commented Dec 4, 2025

You also have to fix the QA job: run black, ruff and pyright.

Never mind about the integration tests issue, it is a false error only linked to the fact the PR originates from a fork.

Copy link
Collaborator

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

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

Code LGTM. Please squash your commits and rebase to have only one commit on top of latest commit of openzim:main branch and I will merge. We don't want merge commits from main branch to feature branch, it is just a mess in Git. I will merge after that.

@aaryansinhaa
Copy link
Author

@benoit74 done!

@benoit74 benoit74 self-requested a review December 10, 2025 13:03
@aaryansinhaa
Copy link
Author

Hey @benoit74 , just wanted to check in, since this PR been open for a while.
Let me know if there are any concerns or changes needed from my side.

@kelson42
Copy link
Contributor

@aaryansinhaa Sorry for the lack on feedback on our side. For the moment @benoit74 is not available, but we don't forget you and he will be back worse case early February to share a feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants