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

[PhoronixBridge] support multipage and embed benchmarks #2522

Merged
merged 5 commits into from Mar 27, 2022

Conversation

Glandos
Copy link
Contributor

@Glandos Glandos commented Mar 23, 2022

The bridge now supports multipage article. In those article, benchmarks from OpenBenchmarking.com are now embedded, either as SVG in <object> tag or as <img>. The second option is useful for feed readers like TT-RSS that disable <object> or <svg> tags for security reasons.

@Bockiii
Copy link
Contributor

Bockiii commented Mar 24, 2022

You can ignore the linting error about donnonsbridge, there was an error in merging another PR. You dont have linting errors with your bridge.

@Glandos
Copy link
Contributor Author

Glandos commented Mar 24, 2022

I ran phpcs prior to submitting. I had one puzzling situation, with the title attribute that was too long. I chose to split with a new line, because using string concatenation brought another phpcs error so… this was unavoidable.


public function collectData(){
$this->collectExpandableDatas('https://www.phoronix.com/rss.php', 15);
$this->collectExpandableDatas('https://www.phoronix.com/rss.php', $this->getInput('n'));
}

protected function parseItem($newsItem){
$item = parent::parseItem($newsItem);
// $articlePage gets the entire page's contents
$articlePage = getSimpleHTMLDOM($newsItem->link);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$articlePage = getSimpleHTMLDOM($newsItem->link);
$articlePage = getSimpleHTMLDOM($newsItem->link);
$articlePage = defaultLinkTo($articlePage, $this->getURI());

Copy link
Contributor

Choose a reason for hiding this comment

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

this should fix the issue that the posts have a category image which's link is relative:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

defaultLinkTo description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and the suggestion. I didn't notice this because TT-RSS was doing the job itself…

github-actions bot added a commit that referenced this pull request Mar 27, 2022
@github-actions
Copy link

🤖 Pull request artifacts

file commit
pr2522-Phoronix-current-context1.html 61b37ca
pr2522-Phoronix-pr-context1.html 61b37ca

github-actions bot added a commit that referenced this pull request Mar 27, 2022
@Bockiii
Copy link
Contributor

Bockiii commented Mar 27, 2022

LGTM, thanks!

@Bockiii Bockiii merged commit fe43537 into RSS-Bridge:master Mar 27, 2022
@Glandos Glandos deleted the phoronix branch March 27, 2022 19:15
Kwbmm pushed a commit to Kwbmm/rss-bridge that referenced this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants