Skip to content
Open
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
1 change: 1 addition & 0 deletions etc/lime-elements.api.md
Comment thread
adrianschmidt marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,7 @@ export type IconSize = 'x-small' | 'small' | 'medium' | 'large';
interface Image_2 {
alt: string;
loading?: 'lazy' | 'eager';
referrerpolicy?: ReferrerPolicy;
src: string;
}
export { Image_2 as Image }
Expand Down
3 changes: 2 additions & 1 deletion src/components/card/card.tsx
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.

limel-file-viewer can also display images. perhaps it could use this feature too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
State,
} from '@stencil/core';
import { Image } from '../../global/shared-types/image.types';
import { ImageTemplate } from '../../util/image.template';
import { Icon, IconName } from '../../global/shared-types/icon.types';
import { isItem } from '../action-bar/is-item';
import { getIconName } from '../icon/get-icon-props';
Expand Down Expand Up @@ -243,7 +244,7 @@ export class Card {

return (
<div class="image-wrapper">
<img src={this.image.src} alt={this.image.alt} loading="lazy" />
<ImageTemplate image={this.image} />
</div>
);
}
Expand Down
5 changes: 2 additions & 3 deletions src/components/chip/chip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Prop,
} from '@stencil/core';
import { Icon, IconName } from '../../global/shared-types/icon.types';
import { ImageTemplate } from '../../util/image.template';
import { Languages } from '../date-picker/date.types';
import { Link } from '../../global/shared-types/link.types';
import { getRel } from '../../util/link-helper';
Expand Down Expand Up @@ -295,9 +296,7 @@ export class Chip implements ChipInterface {
}

if (!isEmpty(this.image)) {
return (
<img src={this.image.src} alt={this.image.alt} loading="lazy" />
);
return <ImageTemplate image={this.image} />;
}

return (
Expand Down
21 changes: 13 additions & 8 deletions src/components/file-viewer/file-viewer.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {
Component,
Element,
Fragment,
h,
Prop,
State,
Expand All @@ -17,6 +18,7 @@ import { FileType, OfficeViewer } from './file-viewer.types';
import { LimelMenuCustomEvent } from '../../components';
import { Email } from '../email-viewer/email-viewer.types';
import { loadEmail } from '../email-viewer/email-loader';
import { ImageTemplate } from '../../util/image.template';

/**
* This is a smart component that automatically detects
Expand Down Expand Up @@ -236,14 +238,17 @@ export class FileViewer {
};

private renderImage = () => {
return [
this.renderButtons(),
<img
src={this.sanitizeUrl(this.fileUrl)}
alt={this.alt}
loading="lazy"
/>,
];
return (
<Fragment>
{this.renderButtons()}
<ImageTemplate
image={{
src: this.sanitizeUrl(this.fileUrl),
alt: this.alt ?? '',
}}
/>
</Fragment>
);
};

private renderVideo = () => {
Expand Down
20 changes: 20 additions & 0 deletions src/components/list-item/list-item.e2e.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -108,5 +108,25 @@ describe('limel-list-item', () => {

const imgEl = root.querySelector('img');
expect(imgEl).not.toBeNull();
expect(imgEl?.hasAttribute('referrerpolicy')).toBe(false);
});

it('forwards referrerpolicy to the rendered image when set', async () => {
const imgSrc =
'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///ywAAAAAAQABAAACAUwAOw==';
const { root, waitForChanges } = await render(
<limel-list-item
text="Pic"
image={{
src: imgSrc,
alt: 'a',
referrerpolicy: 'no-referrer',
}}
></limel-list-item>
);
await waitForChanges();

const imgEl = root.querySelector('img');
expect(imgEl?.getAttribute('referrerpolicy')).toEqual('no-referrer');
});
Comment on lines +114 to 131
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.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Consider adding similar e2e coverage for chip and card components.

List-item now has tests for both referrerpolicy branches, but chip and card lack similar assertions. Since the rendering logic is centralized in ImageTemplate, the risk is low, but adding parallel tests would strengthen confidence that all three components correctly integrate with the template.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/list-item/list-item.e2e.tsx` around lines 114 - 131, Add
end-to-end tests for limel-chip and limel-card mirroring the list-item test to
assert that the image.referrerpolicy is forwarded: create tests named like
"forwards referrerpolicy to the rendered image when set" in the chip and card
e2e files that render <limel-chip> and <limel-card> with image={{ src: ..., alt:
'a', referrerpolicy: 'no-referrer' }}, wait for changes, select the rendered img
via root.querySelector('img') and assert imgEl?.getAttribute('referrerpolicy')
=== 'no-referrer'. Use the same pattern and assertions as the existing list-item
test to ensure ImageTemplate integration is covered for all three components.

});
3 changes: 2 additions & 1 deletion src/components/list-item/list-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { ListSeparator } from '../../global/shared-types/separator.types';
import { CheckboxTemplate } from '../checkbox/checkbox.template';
import translate from '../../global/translations';
import { Languages } from '../date-picker/date.types';
import { ImageTemplate } from '../../util/image.template';

/**
* This components displays the list item.
Expand Down Expand Up @@ -253,7 +254,7 @@ export class ListItemComponent implements ListItem {
return;
}

return <img src={this.image.src} alt={this.image.alt} loading="lazy" />;
return <ImageTemplate image={this.image} />;
};

private renderActionMenu = (actions: Array<MenuItem | ListSeparator>) => {
Expand Down
8 changes: 8 additions & 0 deletions src/components/profile-picture/profile-picture.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@ export class ProfilePicture {
const src = this.getImageSrc();

if (src) {
// Spread-cast: Stencil's `ImgHTMLAttributes` typing omits
// `referrerpolicy`, so the attribute can't be passed directly.
// Tracked upstream at https://github.com/stenciljs/core/issues/6692
// — once that lands, this cast can be removed.
return (
<img
src={src}
Expand All @@ -254,6 +258,10 @@ export class ProfilePicture {
}}
loading="lazy"
onError={this.onImageError}
{...({ referrerpolicy: 'no-referrer' } as Record<
string,
string
>)}
/>
);
}
Expand Down
9 changes: 9 additions & 0 deletions src/global/shared-types/image.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,13 @@ export interface Image {
* - `eager` means that the image will be loaded as soon as possible.
*/
loading?: 'lazy' | 'eager';

/**
* The `referrerpolicy` attribute of the image. Set to `'no-referrer'`
* when `src` points to a third-party service that should not receive
* the originating page URL in the `Referer` header (e.g. external
* favicon or avatar services). When omitted, the attribute is not
* emitted and the browser's default referrer policy applies.
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.

shouldn't we always default to 'no-referrer'?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would be the natural privacy-positive default, but it'd be observable across every ImageTemplate consumer (so a breaking change, not just a feature flip) — and lime-elements is general-purpose, where some consumer could be relying on Referer reaching the image host (hotlink-protected CDNs, server-side correlation).

Leaning toward keeping it opt-in here and letting consumers like lime-crm-components set 'no-referrer' explicitly on the Image objects they construct (the favicon and avatar use cases that motivated this). Happy to revisit as a separate breaking-change PR if we want to make the opinionated call design-system-wide.

*/
referrerpolicy?: ReferrerPolicy;
}
37 changes: 37 additions & 0 deletions src/util/image.template.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { FunctionalComponent, h } from '@stencil/core';
import { Image } from '../global/shared-types/image.types';

interface ImageTemplateProps {
image: Image;
}

/**
* Renders an `Image` as a plain `<img>` element. Centralises the
* attribute forwarding (including the spread-cast workaround for
* `referrerpolicy`) so consumer components do not have to reimplement
* it. Intended for internal use by lime-elements components that
* accept an `Image`-shaped prop.
*
* @param props
* @param props.image - the image to render
* @internal
*/
export const ImageTemplate: FunctionalComponent<ImageTemplateProps> = ({
image,
}) => {
// Spread-cast: Stencil's `ImgHTMLAttributes` typing omits
// `referrerpolicy`, so the attribute can't be passed directly.
// Tracked upstream at https://github.com/stenciljs/core/issues/6692
// — once that lands, this cast can be removed.
const referrerPolicyAttr: { referrerpolicy?: Image['referrerpolicy'] } =
image.referrerpolicy ? { referrerpolicy: image.referrerpolicy } : {};

return (
<img
src={image.src}
alt={image.alt}
loading={image.loading ?? 'lazy'}
{...(referrerPolicyAttr as Record<string, string>)}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
/>
);
};
Loading