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

[PicalaBridge] Add new bridge #2646

Merged
merged 5 commits into from
Apr 13, 2022
Merged

[PicalaBridge] Add new bridge #2646

merged 5 commits into from
Apr 13, 2022

Conversation

Chouchen
Copy link
Contributor

@Chouchen Chouchen commented Apr 12, 2022

Picala is an independant media which communicates only about electrically assisted cycles in french.

This bridge retrieve articles from this site by category :

  • News
  • Economy
  • Tests
  • Usage

@github-actions
Copy link

github-actions bot commented Apr 12, 2022

Pull request artifacts

file last change
Picala-pr-context1 2022-04-13, 10:54:24

@Bockiii
Copy link
Contributor

Bockiii commented Apr 12, 2022

Hi @Chouchen , thanks for the bridge! A few things:

  • Please add a description on what the bridge or the website does, especially for non-english websites.
  • Please remove the comment lines
  • Please move the TYPES const to the parameters array. Moving them to a separate const makes sense for large lists to not clutter up the code but you only have 4 keys, so it should be fine and the additional const just makes it harder to read.
  • Right now, you're only retrieving 4 headlines, nothing else. It would be great if you improve the bridge by actually retrieving the content of the post, maybe the image, the author and the timestamp. Right now, it's a bit bare.

Also, your content isn't working right now.

@Chouchen
Copy link
Contributor Author

Hi and thanks for your comment.

I don't know where to add the description. Maybe the constant?

Picala is an independant media which communicates only about electrically assisted cycles in french.

I removed all comments in the bridge and fixed the content.

About the TYPES constant, I disagree. If it was only used in the PARAMETERS constant, I wouldn't have done this but it's also used in 2 array_search to build the name and description. array_search($this->getInput('type'), self::PARAMETERS[0][0]['type']['values']) is not readable.

@Bockiii
Copy link
Contributor

Bockiii commented Apr 12, 2022

Ah, I see about the types. Then it's okay.

As for the description, I meant the PR description. Just a few lines on what the page is/does and so on.

The content looks better, but I think you're grabbing the wrong image link for articles 2-4. The image you're pulling is some weird, super low pixel preview. You want the one from the srcset, not the src.

Example:

                         <img src="https://picala-static.s3.amazonaws.com/media/CACHE/images/article/2022-03-24/velos-a-paris/b0c4c3bf8c5b3dd54d34c6045ae826f9.jpg"
                              srcset="https://picala-static.s3.amazonaws.com/media/CACHE/images/article/2022-03-24/velos-a-paris/26923ce767b6138cc8d39cbf4b85eb63.jpg 320w"
                              data-src="https://picala-static.s3.amazonaws.com/media/CACHE/images/article/2022-03-24/velos-a-paris/26923ce767b6138cc8d39cbf4b85eb63.jpg"
                              data-srcset="https://picala-static.s3.amazonaws.com/media/CACHE/images/article/2022-03-24/velos-a-paris/26923ce767b6138cc8d39cbf4b85eb63.jpg 320w,
                              https://picala-static.s3.amazonaws.com/media/CACHE/images/article/2022-03-24/velos-a-paris/39d90d848e9260b5faad83c7c57ab5fd.jpg 640w"
                              sizes="(max-width:812px) 90vw, 250px"
                              alt="un vélo à paris sur le boulevard sébastopol"
                              class="lazy article__picture teaser__img"
                         >

You're pulling the src but the srcset is way better.

@Bockiii
Copy link
Contributor

Bockiii commented Apr 13, 2022

Now it's not pulling the image for the latest :) I think it's pulling the webp file, not the jpg.

@Chouchen
Copy link
Contributor Author

I took data-src instead. With srcset, you must parse the last part of the string.

I updated the description here.

@Chouchen
Copy link
Contributor Author

Oh the latest in feed but the first on the website. For some reason, some doesn't have data-src and other have multiples srcset because why not?

OK, time for parsing.

@Bockiii
Copy link
Contributor

Bockiii commented Apr 13, 2022

LGTM. Thanks

@Bockiii Bockiii merged commit ff8ece2 into RSS-Bridge:master Apr 13, 2022
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.

2 participants