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

🐛 Bug: Allcontributors table images should have loading=lazy #1312

Open
3 tasks done
JoshuaKGoldberg opened this issue Feb 15, 2024 · 8 comments
Open
3 tasks done
Assignees
Labels
status: blocked Waiting for something else to be resolved type: bug Something isn't working :(

Comments

@JoshuaKGoldberg
Copy link
Owner

JoshuaKGoldberg commented Feb 15, 2024

Bug Report Checklist

  • I have tried restarting my IDE and the issue persists.
  • I have pulled the latest main branch of the repository.
  • I have searched for related issues and found none that matched my issue.

Expected

Images in the allcontributors table should have loading="lazy" so that they don't slow down initial page load or render.

Actual

I tried loading https://github.com/JoshuaKGoldberg/create-typescript-app in a throttled network connection and noticed a lot of images loading right away. They even seemed to be blocking first page render Yuck.

Additional Info

This might involve fixing on the allcontributors side?

I also don't know if this is possible in GitHub's markdown renderer... but it really should be. If it isn't, I should find a place to request it / upvote an existing request.

@JoshuaKGoldberg JoshuaKGoldberg added type: bug Something isn't working :( status: accepting prs Please, send a pull request to resolve this! labels Feb 15, 2024
@johnnyreilly
Copy link
Sponsor Collaborator

johnnyreilly commented Mar 16, 2024

I very much like this idea (having written about how to do this in Docusaurus and helped make it the default behaviour for Docusaurus I declare an interest 😄 )

Looking at the README.md, it should just work; https://raw.githubusercontent.com/JoshuaKGoldberg/create-typescript-app/main/README.md

However, the proof is in the pudding. I'm going to tweak the README on a branch and see if it works.

@johnnyreilly
Copy link
Sponsor Collaborator

So in tragic news, I don't think this is doable unless the GitHub Markdown rendered changes. Consider:

screenshot of devtools

I tweaked the README.md on the test-loading-lazy branch: https://github.com/JoshuaKGoldberg/create-typescript-app/blob/test-loading-lazy/README.md and hacked in loading="lazy" attributes. Then I went to: https://github.com/JoshuaKGoldberg/create-typescript-app/tree/test-loading-lazy

And as the screenshot of devtools shows, the loading="lazy" were stripped by the GitHub Markdown renderer. So unless that changes, there's not a way forward. How to find out....

@johnnyreilly
Copy link
Sponsor Collaborator

BTW feel free to delete the branch - don't want to clutter. More just wanted to demostrate my findings!

@johnnyreilly
Copy link
Sponsor Collaborator

johnnyreilly commented Mar 16, 2024

The library GitHub uses for Markdown rendering seems to be:

https://github.com/gjtorikian/commonmarker

(If I understand https://github.com/github/markup right)

Potential issue:

Please note that only the first step is covered by this gem - the rest happens on http://GitHub.com. In particular, markup itself does no sanitization of the resulting HTML, as it expects that to be covered by whatever pipeline is consuming the HTML

@johnnyreilly
Copy link
Sponsor Collaborator

So commonmarker wraps the rust library https://github.com/kivikakk/comrak

I've had a little dig through that and don't detect any special processing around images. This makes it seem likely that (as per comment above) GitHub itself does the loading attribute stripping. As such, unless we can influence GitHub in some way, I don't think we can do anything about this issue.

@johnnyreilly johnnyreilly added the status: blocked Waiting for something else to be resolved label Mar 24, 2024
@johnnyreilly
Copy link
Sponsor Collaborator

I've raised a request with GitHub support to discuss whether the loading="lazy" attribute could be allowlisted on GitHub.com.

https://support.github.com/ticket/personal/0/2692240

@johnnyreilly johnnyreilly self-assigned this Mar 24, 2024
@johnnyreilly
Copy link
Sponsor Collaborator

It's a requested feature, this issue cannot advance unless the following is implemented:

https://github.com/orgs/community/discussions/42558

See also: https://gist.github.com/kivikakk/622b5dcf395e26c49e2334f0eb19e6f9

@JoshuaKGoldberg
Copy link
Owner Author

Thanks for the investigation, great finds!

@JoshuaKGoldberg JoshuaKGoldberg removed the status: accepting prs Please, send a pull request to resolve this! label Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: blocked Waiting for something else to be resolved type: bug Something isn't working :(
Projects
None yet
Development

No branches or pull requests

2 participants