Skip to content

Commit

Permalink
[Files] Simplify image component implementation and usage (#145347)
Browse files Browse the repository at this point in the history
## Summary

While working on the Image Embeddable I've struggle with using the image
component, especially making the blur hash part work as I wanted. The
main problems were caused by a custom container around the image which
had custom styling - I had to properly style it separately to fit my
use-case. The other problem was the BlurHash as since it was implemented
as a separate image with custom positioning, I couldn't easily position
it exactly the same as I positioned the original image.

To make Image component easier to use I suggest the following changes. I
also wanted to make the component thinner.

- Use a single <img> tag for the underlying image - both for the src and
for blurhash. This way we can be sure the image and the placeholder are
always position in the same way. To make it work we first set
src=blurhash, and then when it is ready we set a real source. The trick
here is that the browser loads the new src without removing the previous
image.
- The downside is that we lost the logic with `blurDelayExpired`. This
isn't great, but I think we can live with that considering the
simplicity of the new approach. I also reduced the blur box size so that
it calculates a bit more faster, as it would have to do this for every
image now.
- Also, to solve all the edge cases, I had to make the blurred image the
same size as the original image by stretching it back to the original
size. The problem is that I had to make that part of the code async
#145347 (comment)
- The image partially implemented some features from `EuiImage` (like
predefined set of sizes). Instead of keeping the partial implementation
with some code copy paste, I simply suggest we use `EuiImage`
underneath. This make our image a tiny wrapper around `EuiImage` which
also supports blurhash.
- Added a storybook that tests custom sizing with css. 
- We discussed with @jloleysens that maybe we don't needs custom
viewport visibility logic and just use a native browser attribute. The
browsers which don't support it will simply load the image eagerly (only
Safari atm). I think the native approach is fine here and we should
remove the custom code to reduce bug surface.
  • Loading branch information
Dosant authored Nov 18, 2022
1 parent 9cf05a0 commit e3146a0
Show file tree
Hide file tree
Showing 13 changed files with 118 additions and 462 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export const FileCard: FunctionComponent<Props> = ({ file }) => {
`}
meta={file.meta as FileImageMetadata}
src={client.getDownloadHref({ id: file.id, fileKind: kind })}
loading={'lazy'}
/>
) : (
<div
Expand Down
64 changes: 0 additions & 64 deletions src/plugins/files/public/components/image/components/blurhash.tsx

This file was deleted.

61 changes: 0 additions & 61 deletions src/plugins/files/public/components/image/components/img.tsx

This file was deleted.

11 changes: 0 additions & 11 deletions src/plugins/files/public/components/image/components/index.ts

This file was deleted.

67 changes: 27 additions & 40 deletions src/plugins/files/public/components/image/image.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,10 @@

import React from 'react';
import { ComponentStory, ComponentMeta } from '@storybook/react';
import { action } from '@storybook/addon-actions';
import { css } from '@emotion/react';

import { FilesContext } from '../context';
import { getImageMetadata } from '../util';
import { Image, Props } from './image';
import { getImageData as getBlob, base64dLogo } from './image.constants.stories';
import { FilesClient } from '../../types';

const defaultArgs: Props = { alt: 'test', src: `data:image/png;base64,${base64dLogo}` };

Expand All @@ -24,41 +20,38 @@ export default {
component: Image,
args: defaultArgs,
decorators: [
(Story) => (
<FilesContext client={{} as unknown as FilesClient}>
<Story />
</FilesContext>
),
(Story) => {
React.useLayoutEffect(() => {
// @ts-ignore
window.__image_stories_simulate_slow_load = true;
return () => {
// @ts-ignore
window.__image_stories_simulate_slow_load = false;
};
}, []);

return (
<>
<Story />
</>
);
},
],
} as ComponentMeta<typeof Image>;

const Template: ComponentStory<typeof Image> = (props: Props, { loaded: { meta } }) => (
<Image size="original" {...props} meta={meta} ref={action('ref')} />
<Image {...props} meta={meta} />
);

export const Basic = Template.bind({});

export const WithBlurhash = Template.bind({});
WithBlurhash.storyName = 'With blurhash';
WithBlurhash.args = {
style: { visibility: 'hidden' },
};
WithBlurhash.loaders = [
async () => ({
meta: await getImageMetadata(getBlob()),
}),
];
WithBlurhash.decorators = [
(Story) => {
const alwaysShowBlurhash = `img:nth-of-type(1) { opacity: 1 !important; }`;
return (
<>
<style>{alwaysShowBlurhash}</style>
<Story />
</>
);
},
];

export const BrokenSrc = Template.bind({});
BrokenSrc.storyName = 'Broken src';
Expand All @@ -71,26 +64,20 @@ WithBlurhashAndBrokenSrc.storyName = 'With blurhash and broken src';
WithBlurhashAndBrokenSrc.args = {
src: 'foo',
};

WithBlurhashAndBrokenSrc.loaders = [
async () => ({
blurhash: await getImageMetadata(getBlob()),
}),
];

export const OffScreen = Template.bind({});
OffScreen.storyName = 'Offscreen';
OffScreen.args = { onFirstVisible: action('visible') };
OffScreen.decorators = [
(Story) => (
<>
<p>Scroll down</p>
<div
css={css`
margin-top: 100vh;
`}
>
<Story />
</div>
</>
),
export const WithCustomSizing = Template.bind({});
WithCustomSizing.storyName = 'With custom sizing';
WithCustomSizing.loaders = [
async () => ({
meta: await getImageMetadata(getBlob()),
}),
];
WithCustomSizing.args = {
css: `width: 100px; height: 500px; object-fit: fill`,
};
Loading

0 comments on commit e3146a0

Please sign in to comment.