Re-introduce lazy loading #2367 #2450

LamJiuFong · 2024-03-10T14:21:16Z

What is the purpose of this pull request?
Fix #2367

Overview of changes:
Add lazy property to <pic> and <annotate>
Checks if either width or height are given when users want to use lazy loading, a warning will be logged to the console when both are missing

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Add lazy property to pic and annotate

Checklist: ☑️

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

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

Add log warning for missing width and height

codecov · 2024-03-10T14:36:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.04%. Comparing base (f0759d3) to head (92b1c36).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2450      +/-   ##
==========================================
+ Coverage   48.97%   49.04%   +0.07%     
==========================================
  Files         124      124              
  Lines        5254     5262       +8     
  Branches     1112     1118       +6     
==========================================
+ Hits         2573     2581       +8     
  Misses       2376     2376              
  Partials      305      305

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LamJiuFong · 2024-03-10T14:46:58Z

Would like to seek for advice on the log warning:

yucheng11122017 · 2024-03-10T14:48:12Z

Would like to seek for advice on the log warning:

What is your confusion?

LamJiuFong · 2024-03-10T14:49:50Z

Would like to seek for advice on the log warning:

What is your confusion?

Not sure if it can be further improved from a user's point of view.

yucheng11122017 · 2024-03-10T16:07:15Z

Would like to seek for advice on the log warning:

What is your confusion?

Not sure if it can be further improved from a user's point of view.

Ah I see. I think it could be better with the file where it came from so users know exactly where the error is.
Apart from that, it seems ok to me.

LamJiuFong · 2024-03-12T09:51:08Z

Would like to seek for advice on the log warning:

What is your confusion?

Not sure if it can be further improved from a user's point of view.

Ah I see. I think it could be better with the file where it came from so users know exactly where the error is. Apart from that, it seems ok to me.

Sure, I will work on it, thanks for the suggestion!

LamJiuFong · 2024-03-13T14:25:43Z

@yucheng11122017 I have changed the warning message to only specify the file and not the code snippet.

This is because I realised we can only get the code snippet if the <pic> or <annotate> is declared as a <variable> due to how we process the source files.

However, I think it is sufficient to only show the file paths since the users can now have an idea on where to do the changes.
What do you think?

kaixin-hc · 2024-03-13T14:44:08Z

I think having just the filepath but not the code is OK, but can you get the filepath to precede the warning message and be on the same line? Just think its weird that it prints "In ___" afterwards, it looks messy...

LamJiuFong · 2024-03-13T17:42:12Z

@kaixin-hc thanks for the suggestion! I have modified the output as below, not sure if the use of --- will be too messy. I was thinking of separating the filepath and the output message to make it easier to read?

What do you think?

kaixin-hc · 2024-03-14T03:40:17Z

lgtm

docs/userGuide/syntax/pictures.md

kaixin-hc · 2024-03-14T08:21:57Z

packages/core/test/unit/html/NodeProcessor.test.ts

+
+       nodeProcessor.processNode(testNode, new Context(path.resolve(''), [], {}, {}));
+
+       const logMessage = consoleSpy.mock.calls[0];


I think you can write an expect statement like expect(consoleSpy.warn).toHaveBeenCalledWith(undefined); and combine these two

Thanks for the suggestion!
I am thinking of another way to do it expect(consoleSpy).not.toHaveBeenCalled().
It checks if the warn method is not being called at all

kaixin-hc · 2024-03-14T08:26:30Z

packages/core/test/unit/html/NodeProcessor.test.ts

+       const testCode = '<pic scr="" alt="" width="300" lazy></pic>';
+       const testNode = parseHTML(testCode)[0] as MbNode;
+
+       const consoleSpy = jest.spyOn(logger, 'warn');


I'm OK with this I think - though I remember some discussions about spy-on vs mock, where mock tends to be preferred over spying as errors can't be produced from winston.

Here is some additional discussions on stack overflow for your consideration...

Using Jest to mock winston logger

Mocking vs spying

And a related issue in markbind #2099 - feel free to weigh in with your thoughts.

Thanks @luminousleek for sharing these links with me!

I've got an example of how you might mock the logger in #2463

Thank you for the suggestions @kaixin-hc @luminousleek ! Will follow the suggestions and update my code.

Co-authored-by: david <71922282+itsyme@users.noreply.github.com>

kaixin-hc

lgtm

yucheng11122017

Hi @LamJiuFong thanks for the work on this PR!

Personally, I'd prefer not to override the height to maintain the aspect ratio - this would be breaking behaviour in that case. Plus if users want to change the aspect ratio that is their choice as well.
In other words, I think the expected behavior should be: if users enter both height and width, that should be used. Only if either one is missing, do we maintain the height ratio.
wdyt? tagging @kaixin-hc as well

docs/userGuide/syntax/pictures.md

yucheng11122017 · 2024-03-14T13:42:51Z

packages/core/src/html/NodeProcessor.ts

@@ -175,7 +175,6 @@ export class NodeProcessor {

      // log warnings for conflicting attributes
      if (_.has(warnConflictingAtributesMap, node.name)) { warnConflictingAtributesMap[node.name](node); }
-


Please don't have unnecessary changes

Ok, will make the changes!

yucheng11122017 · 2024-03-16T07:16:08Z

packages/vue-components/src/Pic.vue

+      const imgHeight = renderedImg.naturalHeight;
+      const imgWidth = renderedImg.naturalWidth;
+      const aspectRatio = imgWidth / imgHeight;
+      if (this.hasWidth) { // if width is present, overwrite the height (if any) to maintain aspect ratio


Hmmm if this behavior necessary? I think its ok for a user to adjust the aspect ratio as needed.
This might also be breaking behavior in that case.

yucheng11122017 · 2024-03-16T07:34:07Z

Also another thing discussed by @kaixin-hc and I is the behaviour when the width and height is relative eg. 50% or max-width.

Could you check @LamJiuFong and see if lazy loading will cause issues in that case?

itsyme · 2024-03-17T00:27:34Z

packages/core/src/html/NodeProcessor.ts

      default:
        break;
      }
    } catch (error) {
      logger.error(error);
    }
-


I think the newlines were intentional for readability! Perhaps reverting them could prevent any unnecessary change!

Ok, will make the changes!

LamJiuFong · 2024-03-17T06:51:25Z

@yucheng11122017 Thanks for raising them up, I have not thought about the issues you mentioned when I was solving this issue.

Previously, I followed the user guide to enforce the aspect ratio
https://markbind-master.netlify.app/userguide/components/imagesanddiagrams#pictures
But now when I think about it, maybe we should remove this constraint and leave it to the user to decide what they want

According to https://stackoverflow.com/questions/65376615/set-height-of-image-with-100-width-for-cls-cumulative-layout-shift:

it seems like layout shifts won't occur if the widths and heights are relative.

Also, our current implementation of Annotate and Pic doesn't support the case where inputs are proportions eg. width: 50%. I think it is a good idea to support it but I am not sure if it should be tackled altogether in this issue because it looks like a separate feature?

Thanks for commenting!

yucheng11122017 · 2024-03-17T07:17:11Z

Thanks for the detailed response @LamJiuFong

Previously, I followed the user guide to enforce the aspect ratio https://markbind-master.netlify.app/userguide/components/imagesanddiagrams#pictures But now when I think about it, maybe we should remove this constraint and leave it to the user to decide what they want

Ah I see I must have missed this. In that case, I think its ok to continue with this current implementation. We can discuss seperately if we should allow the overriding of aspect ratio in that case. Checking in with @kaixin-hc if you have other opinions?

According to https://stackoverflow.com/questions/65376615/set-height-of-image-with-100-width-for-cls-cumulative-layout-shift:

it seems like layout shifts won't occur if the widths and heights are relative.

Ok great!

Also, our current implementation of Annotate and Pic doesn't support the case where inputs are proportions eg. width: 50%. I think it is a good idea to support it but I am not sure if it should be tackled altogether in this issue because it looks like a separate feature?

Oh hmm I see. I was not aware of this. Thank you for investigating! We can discuss this seperately as well but you can ignore it for now. Thanks!

kaixin-hc · 2024-03-17T07:28:36Z

Thanks for investigating!

I'm good to shelve those questions for another issue and merge this first. (But um. Why the checks failing?)

LamJiuFong · 2024-03-17T07:41:21Z

Thanks for the comments!

I'm good to shelve those questions for another issue and merge this first. (But um. Why the checks failing?)

The logger was imported twice after I merged the master branch. I have fixed the issue.

yucheng11122017

LGTM! Thanks for the work

Jiu Fong Lam added 3 commits March 10, 2024 20:34

Add width and height option to Pic and Annotation

5831792

Add log warning for missing width and height

Improve code quality on lazy loading

4498597

Update test

aca23e6

Jiu Fong Lam and others added 5 commits March 12, 2024 20:13

Merge branch 'master' into 2367-reintroduce-lazy-loading

4f622a4

Update unit tests for lazy loading

a99c6b9

Merge branch 'master' into 2367-reintroduce-lazy-loading

edd443a

Update test

1516b05

Change log warning output and update test

a6c8980

LamJiuFong and others added 2 commits March 14, 2024 12:23

Merge branch 'master' into 2367-reintroduce-lazy-loading

a44621b

Update log warning message

b7c6b0b

itsyme reviewed Mar 14, 2024

View reviewed changes

docs/userGuide/syntax/pictures.md Outdated Show resolved Hide resolved

kaixin-hc reviewed Mar 14, 2024

View reviewed changes

LamJiuFong changed the title ~~[Request Review for Functionality Code] Re-introduce lazy loading #2367~~ [Request Review] Re-introduce lazy loading #2367 Mar 14, 2024

Update docs/userGuide/syntax/pictures.md

7d871f3

Co-authored-by: david <71922282+itsyme@users.noreply.github.com>

kaixin-hc added this to the v5.3.1 milestone Mar 15, 2024

LamJiuFong and others added 2 commits March 16, 2024 03:32

Merge branch 'master' into 2367-reintroduce-lazy-loading

7afad7d

Update tests

d5bdec7

LamJiuFong changed the title ~~[Request Review] Re-introduce lazy loading #2367~~ Re-introduce lazy loading #2367 Mar 16, 2024

kaixin-hc approved these changes Mar 16, 2024

View reviewed changes

Merge branch 'master' into 2367-reintroduce-lazy-loading

16d7de0

yucheng11122017 reviewed Mar 16, 2024

View reviewed changes

itsyme reviewed Mar 17, 2024

View reviewed changes

Merge branch 'master' into 2367-reintroduce-lazy-loading

6180b27

Remove unnecessary changes

92b1c36

yucheng11122017 approved these changes Mar 17, 2024

View reviewed changes

yucheng11122017 merged commit 371228d into MarkBind:master Mar 17, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-introduce lazy loading #2367 #2450

Re-introduce lazy loading #2367 #2450

LamJiuFong commented Mar 10, 2024 •

edited by kaixin-hc

Loading

codecov bot commented Mar 10, 2024 •

edited

Loading

LamJiuFong commented Mar 10, 2024

yucheng11122017 commented Mar 10, 2024

LamJiuFong commented Mar 10, 2024

yucheng11122017 commented Mar 10, 2024

LamJiuFong commented Mar 12, 2024

LamJiuFong commented Mar 13, 2024

kaixin-hc commented Mar 13, 2024

LamJiuFong commented Mar 13, 2024 •

edited

Loading

kaixin-hc commented Mar 14, 2024

kaixin-hc Mar 14, 2024

LamJiuFong Mar 15, 2024

kaixin-hc Mar 14, 2024

luminousleek Mar 15, 2024

LamJiuFong Mar 15, 2024

kaixin-hc left a comment

yucheng11122017 left a comment •

edited

Loading

yucheng11122017 Mar 14, 2024

LamJiuFong Mar 17, 2024 •

edited

Loading

yucheng11122017 Mar 16, 2024

yucheng11122017 commented Mar 16, 2024

itsyme Mar 17, 2024

LamJiuFong Mar 17, 2024

LamJiuFong commented Mar 17, 2024

yucheng11122017 commented Mar 17, 2024

kaixin-hc commented Mar 17, 2024 •

edited

Loading

LamJiuFong commented Mar 17, 2024

yucheng11122017 left a comment


		nodeProcessor.processNode(testNode, new Context(path.resolve(''), [], {}, {}));

		const logMessage = consoleSpy.mock.calls[0];

		@@ -175,7 +175,6 @@ export class NodeProcessor {

		// log warnings for conflicting attributes
		if (_.has(warnConflictingAtributesMap, node.name)) { warnConflictingAtributesMap[node.name](node); }

Re-introduce lazy loading #2367 #2450

Re-introduce lazy loading #2367 #2450

Conversation

LamJiuFong commented Mar 10, 2024 • edited by kaixin-hc Loading

codecov bot commented Mar 10, 2024 • edited Loading

Codecov Report

LamJiuFong commented Mar 10, 2024

yucheng11122017 commented Mar 10, 2024

LamJiuFong commented Mar 10, 2024

yucheng11122017 commented Mar 10, 2024

LamJiuFong commented Mar 12, 2024

LamJiuFong commented Mar 13, 2024

kaixin-hc commented Mar 13, 2024

LamJiuFong commented Mar 13, 2024 • edited Loading

kaixin-hc commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaixin-hc left a comment

Choose a reason for hiding this comment

yucheng11122017 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LamJiuFong Mar 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucheng11122017 commented Mar 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LamJiuFong commented Mar 17, 2024

yucheng11122017 commented Mar 17, 2024

kaixin-hc commented Mar 17, 2024 • edited Loading

LamJiuFong commented Mar 17, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

LamJiuFong commented Mar 10, 2024 •

edited by kaixin-hc

Loading

codecov bot commented Mar 10, 2024 •

edited

Loading

LamJiuFong commented Mar 13, 2024 •

edited

Loading

yucheng11122017 left a comment •

edited

Loading

LamJiuFong Mar 17, 2024 •

edited

Loading

kaixin-hc commented Mar 17, 2024 •

edited

Loading