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

Remove option to lazy-load images #2365

Merged
merged 6 commits into from Sep 10, 2023
Merged

Conversation

jmestxr
Copy link
Contributor

@jmestxr jmestxr commented Sep 8, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain: feature removal

Removes lazy-loading option introduced in PR #1626.
Closes #2364

  • a temporary measure, better solution would be to support lazy-load and scrolling

Overview of changes:

Remove option to lazy load images as it results in scrolling issues (see #2364).

eager prop is removed from <pic> and <annotate> components.

All images are now loaded eagerly.

Anything you'd like to highlight/discuss:

Testing instructions:

Open a browser (preferably a slower one like Firefox).

Notice that page might not scroll to correct section when you go to https://markbind.org/userGuide/components/imagesAndDiagrams.html and click on 'Thumbnails' on the pagenav. Because the previous sections 'Pictures' and 'Annotations' consist of images which are lazy-loaded.

Whereas for this PR, go to https://deploy-preview-2365--markbind-master.netlify.app/userguide/components/imagesanddiagrams and click on 'Thumbnails'. Page should scroll to the correct section 'Thumbnails'.

Proposed commit message: (wrap lines at 72 characters)
Remove option to lazy-load images

Lazy-loading option is introduced through the 'eager' prop in and components.

Images are loaded as scrolling occurs, resulting in inaccurate scrolling as seen in issue #2364.

Let's remove 'eager' prop from and components

All images are now loaded eagerly.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #2365 (cb39323) into master (4dd8d72) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2365   +/-   ##
=======================================
  Coverage   45.16%   45.16%           
=======================================
  Files         123      123           
  Lines        5159     5159           
  Branches     1086     1086           
=======================================
  Hits         2330     2330           
  Misses       2510     2510           
  Partials      319      319           
Files Changed Coverage Δ
packages/vue-components/src/Pic.vue 0.00% <ø> (ø)
...ckages/vue-components/src/annotations/Annotate.vue 0.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tlylt
Copy link
Contributor

tlylt commented Sep 8, 2023

Thank you @jmestxr for the quick fix, could you help raise another issue for reimplementing lazy loading, and mention this PR? This is to keep track of it to find a solution to have both lazy loading and correct scrolling behavior.

When you mention disabling smooth scrolling in #2364 (comment) are you saying if we change headingElement.scrollIntoView({ behavior: 'smooth', block: 'start', inline: 'nearest' }); to not have behavior: smooth, it will also fix the problem with lazy loading?

Lastly a minor point, please use an issue closing keyword mentioned in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword. "Addresses xxx" doesn't work because it is not in the list.


@damithc if we remove this lazy loading feature, it seems like a major version update required. Unless on the account that not many people are using this feature, should we put it as patch/minor, for the version update? Any suggestions?

@jmestxr
Copy link
Contributor Author

jmestxr commented Sep 8, 2023

When you mention disabling smooth scrolling in #2364 (comment) are you saying if we change headingElement.scrollIntoView({ behavior: 'smooth', block: 'start', inline: 'nearest' }); to not have behavior: smooth, it will also fix the problem with lazy loading?

It wouldn't because scrollIntoView only executes when window loads and scrolls to the headingElement with the specified id in the url. After loading, when user clicks on any link, scrolling behavior will still be smooth.

Unless we change scrolling behavior globally, i.e.

* {
  scroll-behavior: auto;
}

@tlylt tlylt changed the title Remove lazy loading of images (#2364) Remove option to lazy-load images Sep 10, 2023
@tlylt tlylt added this to the v5.1.0 milestone Sep 10, 2023
@tlylt tlylt merged commit b342e57 into MarkBind:master Sep 10, 2023
8 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.

PageNav links don't scroll to the correct place in the first try
2 participants