Skip to content

Adopt lazy loading for Pic component#1626

Merged
ang-zeyu merged 5 commits into
MarkBind:masterfrom
kimberlyohq:pic-lazy-loading
Jul 17, 2021
Merged

Adopt lazy loading for Pic component#1626
ang-zeyu merged 5 commits into
MarkBind:masterfrom
kimberlyohq:pic-lazy-loading

Conversation

@kimberlyohq

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • Others, please explain:

Resolves #1559

Overview of changes:
Use loading=lazy attribute to defer loading of images for Pic components

Image is only loaded lazily as seen in the video
https://user-images.githubusercontent.com/60393696/124390276-5bde0880-dd1d-11eb-9cc9-f972f4995ad0.mp4

Anything you'd like to highlight / discuss:
In the issue description, it was mentioned that the height/width resolution might have to be moved accordingly. I tried a few examples and it seems to work as normal on my side. Is this concern regarding the computeWidth function? From what I googled online, the images are loaded once it reaches calculated distance from the viewport. (Source: https://web.dev/browser-level-image-lazy-loading/)
Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Adopt lazy loading for Pic component


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@ang-zeyu

ang-zeyu commented Jul 8, 2021

Copy link
Copy Markdown
Contributor

Is this concern regarding the computeWidth function? From what I googled online, the images are loaded once it reaches calculated distance from the viewport. (Source: https://web.dev/browser-level-image-lazy-loading/)

Thanks for investigating this. My concern was indeed around whether naturalWidth / Height would be populated correctly or at all if using the lazy attribute. Good to hear its unfounded 😀

Other than that, wondering if we should document that lazy loading is used by default and provide a simple prop to disable this behaviour. Sometimes it might be preferable to load all the images in the document in one go for better ux.

@kimberlyohq

Copy link
Copy Markdown
Contributor Author

Other than that, wondering if we should document that lazy loading is used by default and provide a simple prop to disable this behaviour. Sometimes it might be preferable to load all the images in the document in one go for better ux.

Sounds good to me! WDYT about adding a eager prop if user wants to load images eagerly?

@ang-zeyu

ang-zeyu commented Jul 9, 2021

Copy link
Copy Markdown
Contributor

sounds good!

@ang-zeyu ang-zeyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm otherwise! 👍

Comment thread docs/userGuide/syntax/pictures.mbdf Outdated
Comment thread docs/userGuide/syntax/pictures.mbdf Outdated
@ang-zeyu ang-zeyu added this to the v3.0.5 milestone Jul 17, 2021
@ang-zeyu ang-zeyu merged commit b979491 into MarkBind:master Jul 17, 2021
@jmestxr jmestxr mentioned this pull request Sep 8, 2023
11 tasks
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.

Pic component: Adopt lazy loading

2 participants