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

Create cert pages #71

Merged
merged 49 commits into from
Nov 11, 2024

Conversation

angelina-momin
Copy link
Collaborator

@angelina-momin angelina-momin commented Sep 25, 2024

Solves issue: codecheckers/register#37

Related register PR: link

Each entry in the certificate column links to a cert page (see image below) displaying the following:

  • PNG preview of the certificate
  • Details obtained from the codecheck.yml such as page title, author name (with links to their orcid page if available), codechecker name.
  • Abstract of the issue
image

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 25, 2024

@nuest This is still a draft. Need your input on the layout as shown in the image above and the following:
The abstract for some of the papers are not available through the APIs. I have implemented functions to try and retrieve the abstract from both crossref and openalex.

I was thinking of implementing two approaches for the htmls then:

  1. For papers with abstract we can render the htmls as shown above
  2. For papers that we cannot obtain abstract for, we just have the page display the cert and the details from the codecheck.yml

Please let me know what you think. Alternatively we could also not render htmls for those papers without abstracts if youd like or for the sake of consistency across the different cert pages we can also scrap the idea of having abstracts.

@angelina-momin
Copy link
Collaborator Author

Also, do you want the abstract to be displayed (if available) in the case where no certs are displayed (because link to pdf is unavailable) @nuest ?

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 25, 2024

In response to your comment about wanting to directly embed the pdf from the url (codecheckers/register#37 (comment) I have tried implementing it but it fails for the pdfs on zenodo.

The zenodo API only provides links to download the file and not display it. Similarly using the link from zenodo page itself doesnt work either: https://zenodo.org/records/3674056/files/codecheck.pdf. It ends up downloading the file and does not display it.

So at the moment I am still using the approach where I download the pdfs and convert to jpegs to display and deleting the pdf afterwards. Let me know what you think @nuest

@nuest
Copy link
Member

nuest commented Sep 25, 2024

Excellent effort to retrieve the abstracts. If you find the handling of the request and response just a little bit tedious, I'm happy to add dependencies to the respective R packages to make that easier.

  • Yes, please display the abstract even if there is not certificate to be shown - we want search engines to find this page, and the abstract will help with that.
  • Did you already encounter repositories that have multiple PDFs? Is there a way for us to "select" the PDF when creating a certificate or repository, e.g., by using "certificate" in the name? Or will the first alphabetical one be used? We can note this in the codechecker instructions, and they can make sure that the "first one shown in the repo page" will be used by adjusting file names.
  • Re. avoid downloading but display the file: What approach did you try? Maybe we can use PDF.js to load the file and circumvent the download?
  • Re. the conversion to images: the code says PNG, but in you comment you say convert to jpegs - I'd go with the one that causes less trouble with the GitHub repository. When we generate jpegs, the git algorithm will probably always think they are new files? Are the PNGs bitwise the same if you run the conversion multiple times?

Wishes:

  • I think this is no problem because of the licenses of OpenAlex and Crossref, but it would be nice to have a link to the source of the abstract, naming the respective source. "Abstract retrieved from OpenAlex: https://..."
  • Same thinking with the summary of the check - please include that as regular website text below the abstract.

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 25, 2024

@nuest This is still a draft. Need your input on the layout as shown in the image above and the following: The abstract for some of the papers are not available through the APIs. I have implemented functions to try and retrieve the abstract from both crossref and openalex.

I was thinking of implementing two approaches for the htmls then:

  1. For papers with abstract we can render the htmls as shown above
  2. For papers that we cannot obtain abstract for, we just have the page display the cert and the details from the codecheck.yml

Please let me know what you think. Alternatively we could also not render htmls for those papers without abstracts if youd like or for the sake of consistency across the different cert pages we can also scrap the idea of having abstracts.

Do you have a preference on what approach to use in this case where there is no abstract @nuest

@nuest
Copy link
Member

nuest commented Sep 25, 2024

@angelina-momin FYI - I edited my comment above with a wish re. the check summary...

Re. no abstract: second approach, we just don't show it. But we can show the check summary, so there will always be that on the right hand side.

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 25, 2024

@angelina-momin FYI - I edited my comment above with a wish re. the check summary...

Re. no abstract: second approach, we just don't show it. But we can show the check summary, so there will always be that on the right hand side.

Thanks, I implemented these changes for papers with and without available abstracts.
P.S. i am going through your other comments still

image

image

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Sep 25, 2024

  • Did you already encounter repositories that have multiple PDFs? Is there a way for us to "select" the PDF when creating a certificate or repository, e.g., by using "certificate" in the name? Or will the first alphabetical one be used? We can note this in the codechecker instructions, and they can make sure that the "first one shown in the repo page" will be used by adjusting file names.

Yes, I already coded in so that it tries to find "codecheck.pdf" file and so far didnt come across any issues

  • Re. avoid downloading but display the file: What approach did you try? Maybe we can use PDF.js to load the file and circumvent the download?

Let me look into that.

  • Re. the conversion to images: the code says PNG, but in you comment you say convert to jpegs - I'd go with the one that causes less trouble with the GitHub repository. When we generate jpegs, the git algorithm will probably always think they are new files? Are the PNGs bitwise the same if you run the conversion multiple times?

Oeps,sorry my bad. I meant that I convert the pdfs to PNGs and then display them.

  • I think this is no problem because of the licenses of OpenAlex and Crossref, but it would be nice to have a link to the source of the abstract, naming the respective source. "Abstract retrieved from OpenAlex: https://..."

Okay, noted.

  • Same thinking with the summary of the check - please include that as regular website text below the abstract.

What do you mean by this? Summary of the check is obtained from the codecheck.yml file from the repos and I already display the repos links

@angelina-momin
Copy link
Collaborator Author

angelina-momin commented Oct 17, 2024

@nuest Hi, I made changes according to your comments. Took me some time because I am new to html and css.

I made the following changes:

  1. Separate codecheck details and paper details boxes
  2. I made the abstract and summary section scrollable if they exceed a certain length (1st and 2nd screenshot)
  3. In the case where the abstract and the summary are empty strings, they are removed (refer to 4th screenshot)
  4. The cert preview box is always the same height as the content on the right. But as a result the aspect ratios are a bit odd sometimes (refer to 4th screenshot). I am working on fixing that.
  5. The "previous" and "next" buttons darken when the mouse hovers on it.

Let me know what you think or if you have other suggestions

Screenshot 2024-10-17 at 09 46 23
Screenshot 2024-10-17 at 09 51 41
Screenshot 2024-10-17 at 09 51 35

Screenshot 2024-10-17 at 09 47 47

@nuest
Copy link
Member

nuest commented Oct 17, 2024

These screenshots look great!

Only minor details:

  • Can you please turn the "Codecheck report: link" into a visible URL link? I assume behind "Link" is the DOI URL?
  • "Paper title" > just "Title" (consistent with "Abstract" and rely on the headline/box for context)
  • "Paper authors" > just "Authors" (consistent with "Abstract" and rely on the headline/box for context)
  • "Codechecker name" > "Codechecker(s)" - did you test this also with a certificate that has multiple authors?
  • "Codecheck certificate" (in the details) > "Certificate identifier"
  • "Codecheck time" > "Time of check"
  • "Codecheck repo" > "Repository"
  • "Codecheck report" > ... you get the gist by now, not sure what to use best until I understand what "link" does.
  • -"Obtained from Crossref" > suggest to make the text a link, here the fully written URL is a bit too prominent for me.

Sorry, I'm a bit pressed for time so I stayed with the screenshots even when I could have looked some things up in the code.

Good work!

@angelina-momin
Copy link
Collaborator Author

@nuest I implemented your changes as well as adding responsiveness for different screen sizes. Refer to the screenshots in the next comment below. However I am running into issues with the styling which I have not been able to fix. If you look at the template_base.md you'll see that I load bootstrap 5.3.3. This results in different styling in the postfix. Not sure where to make the appropriate changes to fix the issues.

@angelina-momin
Copy link
Collaborator Author

Different scaling for large medium and small screens

Screenshot 2024-11-05 at 15 46 22
Screenshot 2024-11-05 at 15 46 41

Screenshot 2024-11-05 at 15 47 15

@nuest
Copy link
Member

nuest commented Nov 5, 2024

@angelina-momin The differences in styling seem negligible to me. I didn't even expect responsiveness would be on the radar.

I suggest to move forward and leave things as they are, I will debug with more time after you are finished with your contributions.

I'm happy to merge!

@angelina-momin angelina-momin marked this pull request as ready for review November 11, 2024 16:35
@angelina-momin angelina-momin merged commit b033d0a into codecheckers:master Nov 11, 2024
3 checks passed
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