Add album display option - #29
Conversation
|
@arraypress what do you think? |
|
Thanks for this — it's a careful PR. I'm going to pass on it for core, though, and I want to give you the actual reasons rather than a vague "out of scope". 1. It breaks the size ceiling. I measured both branches, gzipped:
The documented ceiling is ~14KB (14,336 B). This lands 22 bytes over it and consumes all 139 B of remaining headroom to get there. That budget is real — the option-normalization pass in 1.25.0 cost +880 B and was a deliberate, measured trade for turning two hard crashes into warnings. A third metadata line isn't in that category. 2. One boolean here is fifteen packages of work. Types flow through the family via 3. Two lines is the design, not an omission. There's also a layout wrinkle: What I'd suggest instead
More useful to you than any of the above I noticed Gutenberg carries
If you tell me what else is in that patch file, I'll upstream it and you can delete it. That's worth more to both of us than And if the Playlist block genuinely needs album display, say so — that's a much stronger case than the diff alone makes, and it changes my answer. Separately, and genuinely unrelated to any of the above — no urgency, and I'm only asking because you'd know who to point me at. The library isn't in the External Libraries list on the 7.1 credits page, though it's bundled in the Playlist block. Mediabunny, getID3 and MediaElement.js from that same stack are all listed. Is that just an oversight in the release process, or is there somewhere I should file it? |
Sorry about this, I had missed it. Where are you seeing those credits? I'll get it updated or find someone who can. |
|
The need for an album setting came about from WordPress/gutenberg#81876, but you're right that we can just use the artist field for it and add the album name in (as that PR does). I wanted to run this idea past you first though, before we commit to using the artist field, so we are aligned on that approach - if you added an album field later then we'd want to change the approach which would create complications. Happy to stick with the approach as outlined above :) |
|
No apology needed — thanks for picking it up. It's in the meta repo rather than core: [ 'Waveform Player', 'https://waveformplayer.com' ],Looks like a single missed line rather than anything systemic — That repo takes PRs directly, so it should be a quick one. Appreciate you sorting it. |
|
In terms of the patch on 1.26.0, your analysis is correct, those are the two remaining concerns which we need to patch, there's nothing else in there. It would be great if we can remove that patch file! |
|
Really glad you asked before committing to it — that's exactly the right instinct, and it saves us both a migration later. So, to be unambiguous: no album field is coming to the player's info block. Build against The honest reason is less about bytes than my first reply made it sound. I'm a musician as well as the person maintaining this, and what I keep coming back to is that an album is a container, not a property of a track. Render it per-row and you end up printing the same string down the whole list. It's worth looking at how the big services handle it. In an album view — Tidal, Spotify, Apple Music — there's no album column: just number, title, duration, because the album is the page you're already on. Album only earns a column in a mixed view, a playlist or search results or liked songs, where it's genuinely telling one row from another. That's the distinction I'd bring to #81876. From the screenshot that tracklist looks like an album view, so "Show album name" would print the same value on every row for a line of vertical space per track. If a playlist really does mix releases then the column has a job — but even then the player doesn't need it, since it's showing one track at a time and the row it came from already said which release it's from. So: artist field for the combined string, as your PR already does. Happy to be aligned on that :) And on the patch — good news, that's a small enough surface. I'll get both into the next release: drop the |
|
I get what you're saying about the album field (I'm a musician as well!). However I was thinking about a use case where someone might build a mixtape style playlist that contains multiple albums. We already have the album field so it seems strange that there's currently no option to display it. Feel free to comment on the Gutenberg PR, by the way, we're happy to get input from everyone :) |
|
Closing this as it's served its purpose. |
|
I added the credits here: WordPress/wordpress.org#825 |
|
Perfect, thanks. I gave you a shoutout here btw: https://docs.waveformplayer.com/news/wordpress-7-1/ |
2 participants
Adds a
showAlbumoption anddata-show-albumattribute to render album names in the info block while preserving metadata-only default behavior. Wires album updates throughloadTrack, external request event details, TypeScript declarations, README examples, CSS, and generated dist bundles. Adds coverage for data parsing, normalization, safe album rendering, request payloads, and track-swap album updates/removal. Verified withnpm testandnpm run build.