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

Add links to now playing popup #3700

Merged
merged 3 commits into from May 7, 2023
Merged

Add links to now playing popup #3700

merged 3 commits into from May 7, 2023

Conversation

yayuyokitano
Copy link
Member

Adds links to the now playing popup for track, artist, album, album artist, playcount (leading to track in users library).

Also does a bit of general split up of the now playing component.

@yayuyokitano yayuyokitano added core This issue or pull request is related to the extension core new-feature For PRs that add new functionality labels May 5, 2023
Copy link
Member

@inverse inverse left a comment

Choose a reason for hiding this comment

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

LGTM - nice one!

if (!artist) {
return '';
}
return `https://www.last.fm/music/${encodeURIComponent(artist)}`;
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing we don't need to handle localisation here as lastfm should redirect?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah thats a good spot actually. No lastfm does not redirect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at v2 - it seems web scrobbler never localized urls.
Since the main purpose here is to restore functionality i think localization of URLs should be a separate PR.
Did some simplification regardless, and fixed one pretty huge bug i found that i introduced in #3671 that caused applying an edit to not actually exit the edit state.

@yayuyokitano yayuyokitano requested a review from inverse May 5, 2023 21:09
@yayuyokitano
Copy link
Member Author

Also just to make it clear, album and album artist are not grabbed from metadata because it is unreliable method of getting link for those fields. Track and artist are reliable.

@yayuyokitano
Copy link
Member Author

Reverted the link change – reverting this allows us to change the baseurl based on localization more easily later

@yayuyokitano yayuyokitano merged commit ecc1570 into web-scrobbler:master May 7, 2023
8 checks passed
@yayuyokitano yayuyokitano deleted the anchors branch May 8, 2023 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core This issue or pull request is related to the extension core new-feature For PRs that add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants