-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Add Harmony release action links to MusicBrainz pages #45
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
base: main
Are you sure you want to change the base?
Conversation
should i add the built script to the PR? |
Hi, my time is limited currently, so I won't get to review this PR before the weekend. However, I wanted to let you know early that there is already another userscript which does a similar task. From a brief look, it seems that your script supports more MB pages, so it might be worth trying to integrate this into the other userscript, if it fits there. |
Thanks for letting me know! No worries at all—take your time reviewing the PR, I understand you have other priorities. I hadn’t realized there was another userscript doing something similar. My intent was just to convert your bookmarklet into a script and extend it to more MB pages. I’d prefer to keep it here since the project structure is well-organized with the build scripts, and I’ve reused some of your functions to avoid duplication. Also, since I’ve already put together the PR, integrating it into the other userscript would require reworking things a bit due to the different approaches. I also think that in the long run, having a centralized place for MusicBrainz userscripts, rather than everyone maintaining separate repositories, would make things easier to manage and discover. But of course, I’m open to discussion if you feel otherwise! Let me know your thoughts when you have time. |
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 finally built and tested the userscript, it works fine.
Personally I consider it too invasive to have release actions links for every release on artist and release group pages, the probability that I'm going to use them is probably limited.
Most people would probably go through their favourite artist's releases one time and use the release actions, afterwards they are unlikely to be used again. But maybe I am just not the target audience of this userscript 😀
That said, I would still accept the userscript in this repo if you are willing to maintain it. I can probably fix simple issues (if the website changes, for example), but it's hard for me to guarantee maintainenance of a script which I'm not using myself.
Now some actual feedback about the script itself:
What do you think about injecting the icon link after the release title? It feels a bit cleaner to me to have a secondary action after the title and not next to the entity icon (on release pages) or the cover art indicator icon (in release tables).
I would like to avoid inline style attributes, can you try to style the icon with CSS classes? Ideally making use of one of the existing CSS rules from the website, but if no suitable rules exist, we can always inject our own minimal stylesheet (like here).
- user can turn off script on pages if needed - css is injected as asked - icon is added after the title as suggested - runs only on digital releases by default
Thanks for taking the time to build, test, and provide feedback on the userscript! I really appreciate it. I understand your point about the icons on artist/RG pages potentially being less frequently used after an initial pass. To address it I have added a key value to later turn it off. I'm happy to maintain the script as I use it, also as it might still be useful for users who frequently interact with Harmony or revisit those pages. If maintenance becomes an issue down the line, we can definitely reassess. Regarding your specific feedback on the script itself:
I've pushed these changes. Let me know what you think! Thanks again for the review and guidance. |
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.
Thank you very much for the latest improvements, especially the persistent configuration values. I really like your attention to details and appreciate your patience, sorry that it took me so long to review this again.
Just one last thing remains to be fixed and you have won a new user 😀
harmonyLink.href = `${DEFAULT_HARMONY_BASE_URL}${mbid}`; | ||
harmonyLink.target = "_blank"; | ||
harmonyLink.rel = "noopener noreferrer"; | ||
harmonyLink.title = `View Harmony Actions for ${mbid} (opens in new tab)`; |
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.
harmonyLink.title = `View Harmony Actions for ${mbid} (opens in new tab)`; | |
harmonyLink.title = `View Harmony Release Actions (opens in new tab)`; |
I don't think it is necessary to include the MBID in the tooltip, it is not important for the user (and in case it is, the full URL is usually shown in the browser's status bar/tooltip).
|
||
// --- Format Check --- | ||
if (digitalOnly) { | ||
const formatCell = parentRow.querySelector("td:nth-of-type(4)"); // 4th column is Format |
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 check is a bit unreliable and doesn't work in all scenarios. For example, there is a popular userscript which adds an additional column, shifting the format into the fifth column.
Maybe you could determine the position of the "Format" header cell once for every table and use that index here?
} | ||
} | ||
}); | ||
// console.log(`Harmony Link Script: Processed table "${tableBodySelector}". Added ${addedCount} icons.`); |
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.
Instead of commenting out these logger statements, you can use console.debug
. The bundler will remove them automatically for production builds (build
task) and only preserve them for debug builds (debug-build
task).
Co-authored-by: David Kellner <[email protected]>
sorry for the late reply I have applied your suggestions as well as added logic to get format column from instead of hard coding the value. It works with the userscript you linked. However it worked without the patch as well! I think it must be because my script ran before the other one could add a new column, and hence i could not test my scripts failure before this patch. Regardless, it should now work with the new patch with any number of columns being added/removed. |
This userscript adds a convenient link to the Harmony website directly from MusicBrainz pages, making it easier to jump to Harmony's release-specific actions.
Motivation:
Currently, navigating from a MusicBrainz release to its corresponding Harmony actions page requires manually copying the MBID and constructing the URL. This script automates that process by adding a clickable icon.
Functionality:
On single release pages, the icon is placed in the main page header next to the release title.
On release group and artist releases pages, icons are added next to each release listed in the main table.
Clicking any icon opens the corresponding Harmony release actions URL (https://harmony.pulsewidth.org.uk/release/actions?release_mbid=...) in a new browser tab.