-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enable adding albums to playlists #1568
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1568 +/- ##
==========================================
+ Coverage 71.82% 72.07% +0.24%
==========================================
Files 364 362 -2
Lines 6740 6743 +3
Branches 484 496 +12
==========================================
+ Hits 4841 4860 +19
+ Misses 1502 1477 -25
- Partials 397 406 +9 ☔ View full report in Codecov by Sentry. |
Thanks for adding this, and for the test coverage. I'm not sure if that's the best way to handle it here (the track popup is at least self-contained), maybe it would be better to make that whole menu its own component, with the input dialog and whatnot. For this PR, it's alright, so I'll merge it once the CI passes. |
I thought about extracting the pop-up to a separate component, but I decided to first add the functionality and leave the refactoring decision to you. |
Not really, I do that whenever I feel like, or when I'm working on a large feature (like the local library scanner) and keep seeing untidy code. Sometimes instead of working on features I just look for stuff I neglected to do earlier and work on it. If I do any refactoring though, I make sure that area has decent test coverage to avoid regressions. Any such work is welcome but I realize it can be boring by itself, so usually I do it myself. |
So would you accept refactoring PRs? I usually try to strictly separate refactoring from adding new features, to avoid regressions, specially when I'm not very familiar with the code base. |
Sure, I would be very glad to see that. Typically I'm the only one doing that. |
Added an "Add album to playlist" entry to albums' menu.
Allows adding the album tracks to either an existing or a new playlist.
See #1511