Skip to content

[PhoronixBridge] support multipage and embed benchmarks#2522

Merged
Bockiii merged 5 commits intoRSS-Bridge:masterfrom
Glandos:phoronix
Mar 27, 2022
Merged

[PhoronixBridge] support multipage and embed benchmarks#2522
Bockiii merged 5 commits intoRSS-Bridge:masterfrom
Glandos:phoronix

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.

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
IAM-marco pushed a commit to IAM-marco/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.

2 participants