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

Fix Idagio connector #3892

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

LibertyFrog
Copy link
Contributor

Describe the changes you made
Fixed the Idagio connector not recognizing when a track is playing.

Additional context
The Idagio connector was not scrobbling music. The isPlaying check expects the play button label to equal "Pause" when a track is playing which is also what is visible in the DOM, however they use a text-transform: uppercase CSS styling on it.
I call toUpper on the found text, so it doesn't break again should they remove the styling.

@hornbuckle
Copy link
Contributor

If the text inside the element is "Pause," CSS text-transform shouldn't have any effect on the connector script since it would only affect how the browser renders the text.
Can you provide a link to show the issue you're trying to fix?

@inverse inverse added fixed patch-change For patch changes labels Jun 15, 2023
@LibertyFrog
Copy link
Contributor Author

If the text inside the element is "Pause," CSS text-transform shouldn't have any effect on the connector script since it would only affect how the browser renders the text. Can you provide a link to show the issue you're trying to fix?

That is not the behaviour I'm observing. As for a link: go to https://app.idagio.com and click the play icon on any album tile. When I do it the extension doesn't start scrobbling and when I look at the State object I see the isPlaying property is still false.

This is what I see in the DOM for the pause button selector, "Pause":
pause-in-dom

This is the CSS applying the text-transform:
css-class

I added logging to the isPlaying getter (without the toUpper I added later):
log-text

This is the log output, all uppercase "PAUSE":
log-value

This is the log output after toggling text-transform in the browser dev tools (no other change):
css-no-text-transform
log-value-no-text-transform

@hornbuckle
Copy link
Contributor

Thank you for providing that explanation with screenshots too! It's odd that they would even apply text-transform to an element that isn't actually visible. But now I see the reason why it is behaving this way: getTextFromSelectors function is using innerText which, unlike textContent, is aware of any styling applied to the element. Even though the element isn't visible to a user, it's still technically being displayed in the browser and inheriting the text-transform styling applied to it.

@hornbuckle
Copy link
Contributor

While it's being updated, one other note on this particular connector to prevent future breakage is that the random characters in the classes may change when an update is pushed to the site, so it would be advisable to target them a bit different. For example, .player-PlayerInfo__recordingInfo--15VMv should be targeted as [class^=player-PlayerInfo__recordingInfo--] so it will still work even if those random characters on the end change. (See the Mixcloud connector, for example: https://github.com/web-scrobbler/web-scrobbler/blob/master/src/connectors/mixcloud.ts )

@inverse
Copy link
Member

inverse commented Jul 1, 2023

Any updates on this one?

@yayuyokitano
Copy link
Member

Merging this. Improvement suggested by hornbuckle can be applied in a separate PR.

@yayuyokitano yayuyokitano merged commit 096a6e0 into web-scrobbler:master Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-change For patch changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants