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

Re-introduce lazy loading #2367 #2450

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions docs/userGuide/syntax/annotations.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ The x and y coordinates of each Annotate Point are relative to the image and are
<variable name="highlightStyle">html</variable>
<variable name="code">

<annotate src="../../images/annotateSampleImage.png" width="500" alt="Sample Image">
<annotate src="../../images/annotateSampleImage.png" width="500" alt="Sample Image" lazy>
<!-- Minimal Point -->
<a-point x="25%" y="25%" content="This point is 25% from the left and 25% from the top" />
<!-- Customize Point Size (default size is 40px) -->
Expand Down Expand Up @@ -191,11 +191,12 @@ Here we showcase some use cases of the Annotate feature.
This is effectively the same as the options used for the [picture](#pictures) component.

| Name | Type | Default | Description |
| ------ | --------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|--------| --------- | ------- |-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| alt | `string` | | **This must be specified.**<br>The alternative text of the image. |
| src | `string` | | **This must be specified.**<br>The URL of the image.<br>The URL can be specified as absolute or relative references. More info in: _[Intra-Site Links]({{baseUrl}}/userGuide/formattingContents.html#intraSiteLinks)_ |
| height | `string` |`''`| The height of the image in pixels. |
| width | `string` |`''`| The width of the image in pixels.<br>If both width and height are specified, width takes priority over height. It is to maintain the image's aspect ratio. |
| lazy | `boolean` | false | The `<annotate>` component lazy loads if this attribute is specified.<br>**Either the height or width should be specified to avoid layout shifts while lazy loading images.** |

</div>

Expand Down
5 changes: 3 additions & 2 deletions docs/userGuide/syntax/pictures.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<include src="codeAndOutput.md" boilerplate >
<variable name="highlightStyle">html</variable>
<variable name="code">
<pic src="https://markbind.org/images/logo-lightbackground.png" width="300" alt="Logo">
<pic src="https://markbind.org/images/logo-lightbackground.png" width="300" alt="Logo" lazy>
MarkBind Logo
</pic>
</variable>
Expand All @@ -18,11 +18,12 @@ alt | `string` | | **This must be specified.**<br>The alternative text of the im
height | `string` | | The height of the image in pixels.
src | `string` | | **This must be specified.**<br>The URL of the image.<br>The URL can be specified as absolute or relative references. More info in: _[Intra-Site Links]({{baseUrl}}/userGuide/formattingContents.html#intraSiteLinks)_
width | `string` | | The width of the image in pixels.<br>If both width and height are specified, width takes priority over height. It is to maintain the image's aspect ratio.
lazy | `boolean` | false | The `<pic>` component lazy loads if this attribute is specified.<br>**Either the height or width should be specified to avoid layout shifts while lazy loading images.**

<div id="short" class="d-none">

```html
<pic src="https://markbind.org/images/logo-lightbackground.png" width="300" alt="Logo">
<pic src="https://markbind.org/images/logo-lightbackground.png" width="300" alt="Logo" lazy>
MarkBind Logo
</pic>
```
Expand Down
14 changes: 14 additions & 0 deletions packages/core/src/html/NodeProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,20 @@ export class NodeProcessor {
*/
if (!_.has(node.attribs, 'v-pre')) { node.attribs['v-pre'] = ''; }
break;
case 'pic':
case 'annotate':
if (_.has(node.attribs, 'lazy')
&& !(_.has(node.attribs, 'width') || _.has(node.attribs, 'height'))) {
const filePath = context.callStack.length > 0 ? context.callStack[context.callStack.length - 1]
: context.cwf;
logger.warn(
`${filePath} --- `
+ 'Both width and height are not specified when using lazy loading in the file and'
+ ' it might cause shifting in page layouts. '
+ 'To ensure proper functioning of lazy loading, please specify either one or both.\n',
);
}
break;
default:
break;
}
Expand Down
65 changes: 64 additions & 1 deletion packages/core/test/unit/html/NodeProcessor.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import path from 'path';
import cheerio from 'cheerio';
import htmlparser from 'htmlparser2';
import * as testData from './NodeProcessor.data';
import { expect } from '@jest/globals';
import * as logger from '../../../src/utils/logger';
import * as testData from './NodeProcessor.data';
import { Context } from '../../../src/html/Context';
import { shiftSlotNodeDeeper, transformOldSlotSyntax } from '../../../src/html/vueSlotSyntaxProcessor';
import { getNewDefaultNodeProcessor } from '../utils/utils';
Expand Down Expand Up @@ -139,6 +140,68 @@ test('processNode processes dropdown with header slot taking priority over heade
expect(warnSpy).toHaveBeenCalledWith(testData.PROCESS_DROPDOWN_HEADER_SLOT_TAKES_PRIORITY_WARN_MSG);
});

test('processNode does not log warning when lazy pic has width or height',
() => {
const nodeProcessor = getNewDefaultNodeProcessor();

const testCode = '<pic scr="" alt="" width="300" lazy></pic>';
const testNode = parseHTML(testCode)[0] as MbNode;

const consoleSpy = jest.spyOn(logger, 'warn');
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

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

Thanks @luminousleek for sharing these links with me!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


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

expect(consoleSpy).not.toHaveBeenCalled();
});

test('processNode does not log warning when lazy annotate has width or height',
() => {
const nodeProcessor = getNewDefaultNodeProcessor();

const testCode = '<annotate scr="" alt="" height="300" lazy></annotate>';
const testNode = parseHTML(testCode)[0] as MbNode;

const consoleSpy = jest.spyOn(logger, 'warn');

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

expect(consoleSpy).not.toHaveBeenCalled();
});

test('processNode logs warning when lazy pic no width and height',
() => {
const nodeProcessor = getNewDefaultNodeProcessor();

const testCode = '<pic scr="" alt="" lazy></pic>';
const testNode = parseHTML(testCode)[0] as MbNode;

const consoleSpy = jest.spyOn(logger, 'warn');

nodeProcessor.processNode(testNode, new Context('testpath.md', [], {}, {}));

expect(consoleSpy).toHaveBeenCalledWith('testpath.md --- '
+ 'Both width and height are not specified when using lazy loading in the file and'
+ ' it might cause shifting in page layouts. '
+ 'To ensure proper functioning of lazy loading, please specify either one or both.\n');
});

test('processNode logs warning when lazy annotate no width and height',
() => {
const nodeProcessor = getNewDefaultNodeProcessor();

const testCode = '<annotate scr="" alt="" lazy></annotate>';
const testNode = parseHTML(testCode)[0] as MbNode;

const consoleSpy = jest.spyOn(logger, 'warn');

nodeProcessor.processNode(testNode, new Context('testpath.md', [], {}, {}));

expect(consoleSpy).toHaveBeenCalledWith('testpath.md --- '
+ 'Both width and height are not specified when using lazy loading in the file and'
+ ' it might cause shifting in page layouts. '
+ 'To ensure proper functioning of lazy loading, please specify either one or both.\n');
});

test('markdown coverts inline colour syntax correctly', async () => {
const nodeProcessor = getNewDefaultNodeProcessor();
const indexPath = 'index.md';
Expand Down
30 changes: 23 additions & 7 deletions packages/vue-components/src/Pic.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
:src="src"
:alt="alt"
:width="computedWidth"
:height="computedHeight"
:loading="computedLoadType"
class="img-fluid rounded"
@load.once="computeWidth"
@load.once="computeWidthAndHeight"
/>
<span class="image-caption">
<slot></slot>
Expand Down Expand Up @@ -35,6 +37,10 @@ export default {
type: String,
default: '',
},
lazy: {
type: Boolean,
default: false,
},
addClass: {
type: String,
default: '',
Expand All @@ -53,20 +59,30 @@ export default {
}
return this.widthFromHeight;
},
computedHeight() {
return this.heightFromWidth;
},
computedLoadType() {
return this.lazy ? 'lazy' : 'eager';
},
},
data() {
return {
widthFromHeight: '',
heightFromWidth: '',
};
},
methods: {
computeWidth() {
if (!this.hasWidth && this.hasHeight) {
const renderedImg = this.$refs.pic;
const imgHeight = renderedImg.naturalHeight;
const imgWidth = renderedImg.naturalWidth;
const aspectRatio = imgWidth / imgHeight;
computeWidthAndHeight() {
const renderedImg = this.$refs.pic;
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

this.heightFromWidth = Math.round(toNumber(this.width) / aspectRatio).toString();
} else if (this.hasHeight) {
this.widthFromHeight = Math.round(toNumber(this.height) * aspectRatio).toString();
this.heightFromWidth = this.height;
}
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ exports[`Annotation with customised annotation points 1`] = `
>
<img
class="annotate-image"
height=""
loading="eager"
src="./annotateSampleImage.png"
width=""
/>
Expand Down Expand Up @@ -92,6 +94,8 @@ exports[`Annotation with different visual annotation points 1`] = `
>
<img
class="annotate-image"
height=""
loading="eager"
src="./annotateSampleImage.png"
width=""
/>
Expand Down Expand Up @@ -380,6 +384,8 @@ exports[`Annotation with markdown in header, content and label 1`] = `
>
<img
class="annotate-image"
height=""
loading="eager"
src="./annotateSampleImage.png"
width=""
/>
Expand Down
32 changes: 24 additions & 8 deletions packages/vue-components/src/annotations/Annotate.vue
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
:src="src"
:alt="alt"
:width="computedWidth"
:height="computedHeight"
:loading="computedLoadType"
class="annotate-image"
@load.once="getWidth"
@load.once="computeWidthAndHeight"
/>
<div style="top: 0; left: 0; height: 0;">
<slot></slot>
Expand Down Expand Up @@ -35,6 +37,10 @@ export default {
type: String,
default: '',
},
lazy: {
type: Boolean,
default: false,
},
addClass: {
type: String,
default: '',
Expand All @@ -53,20 +59,30 @@ export default {
}
return this.widthFromHeight;
},
computedHeight() {
return this.heightFromWidth;
},
computedLoadType() {
return this.lazy ? 'lazy' : 'eager';
},
},
data() {
return {
widthFromHeight: '',
heightFromWidth: '',
};
},
methods: {
getWidth() {
if (!this.hasWidth && this.hasHeight) {
const renderedImg = this.$refs.pic;
const imgHeight = renderedImg.naturalHeight;
const imgWidth = renderedImg.naturalWidth;
const imageAspectRatio = imgWidth / imgHeight;
this.widthFromHeight = Math.round(toNumber(this.height) * imageAspectRatio);
computeWidthAndHeight() {
const renderedImg = this.$refs.pic;
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
this.heightFromWidth = Math.round(toNumber(this.width) / aspectRatio).toString();
} else if (this.hasHeight) {
this.widthFromHeight = Math.round(toNumber(this.height) * aspectRatio).toString();
this.heightFromWidth = this.height;
}
},
},
Expand Down
Loading