-
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
[EconomistBridge] Add new bridge #1067
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!
Find below a few points for consideration.
In this case it also makes sense to add the getIcon
function, so feed aggregators can get the favicon:
public function getIcon() {
return 'https://www.economist.com/sites/default/files/econfinal_favicon.ico';
}
Thanks for the review, I've added all changes. |
Thanks for the updates. One small change regarding the item limit: if (count($this->items) >= 10)
break;
$this->items[] = $item; This checks the limit before adding items, but it should be checked afterwards: $this->items[] = $item;
if (count($this->items) >= 10)
break; |
The limit issue is fixed |
Perfect ! Thanks ! |
* [EconomistBridge] Added new bridge
Adds a new bridge for the latest stories on The Economist.
Filters out the "Subscribe to Newsletter" form and some links at the bottom of each article.