-
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
[QPlayBridge]: New Bridge #1118
Conversation
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.
Thanks for the PR! Just small comments before we can merge !
Thanks for the review, commited those changes. |
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.
Hey, nice bridge. Find below two suggestions
It's still erroring out here:
The getIcon
function is optional and should return a direct link to the favicon. Please try to avoid loading contents in this function. The following code should work fine.
public function getIcon() {
return 'https://s3.amazonaws.com/unode1/assets/4957/r3T9Lm9LTLmpAEX6FlSA_apple-touch-icon.png';
}
If the location of the icon changes, leaving the function out is probably the best option.
Remove fetching data on `getIcon`
Appease Travis
Many thanks for the review, commited your (thoughtful) recommendations. |
Thanks for taking in account @logmanoriginal's requests ! Merged ! |
* [QPlayBridge]: New Bridge
Yet another super specific bridge, but this site uses the uscreen platform, it might be used as a template for other sites.
The other public sites I tested hide all this from anonymous users, so I scrapped the generic version of this.