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

[Emotion] Convert EuiImage #5969

Merged
merged 53 commits into from
Jul 20, 2022
Merged
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
efd2852
Initial conversion
elizabetdev Jun 15, 2022
3a1244d
Fixing close button color
elizabetdev Jun 15, 2022
f558663
Removing unused sass files
elizabetdev Jun 16, 2022
756b90c
Adding CL
elizabetdev Jun 20, 2022
1ae8892
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev Jun 20, 2022
2247ac9
Better conditions and styles
elizabetdev Jun 21, 2022
ecb63f5
Adding _imageMargins style function
elizabetdev Jun 22, 2022
e062665
WIP
elizabetdev Jun 22, 2022
7f6c8e5
Improving `_imageWidth`
elizabetdev Jun 22, 2022
016bd62
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev Jun 24, 2022
c801e40
Splitting component in `EuiImage` and `EuiImageWrapper`.
elizabetdev Jun 24, 2022
cba8d5a
Improving `EuiText` image styles
elizabetdev Jun 28, 2022
e0e075b
Better styles
elizabetdev Jun 29, 2022
4cb1893
Adding `shouldRenderCustomStyles`
elizabetdev Jun 29, 2022
50ef8b2
Renaming styles
elizabetdev Jun 30, 2022
2268d5a
splitting in more components
elizabetdev Jun 30, 2022
bddf55f
Better components split
elizabetdev Jun 30, 2022
d85a683
Fixing caption hover styles
elizabetdev Jun 30, 2022
c4de638
Omit onClick from EuiImageWrapperProps and EuiImageFullScreenWrapperP…
chandlerprall Jun 30, 2022
0bf23ee
Reorganizing imports and types
elizabetdev Jun 30, 2022
f7cff69
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev Jul 4, 2022
3dcf262
Better styles
elizabetdev Jul 4, 2022
4d9b546
Adding `commonImgProps` and `commonWrapperProps`
elizabetdev Jul 4, 2022
51fa9e6
CL
elizabetdev Jul 5, 2022
e52dad8
CL again
elizabetdev Jul 5, 2022
62dbd45
Adding `euiImageFullScreenWrapper` className
elizabetdev Jul 5, 2022
2ee3c50
Adding `Breaking changes` CL entry
elizabetdev Jul 5, 2022
6a6cf5c
Adding `logicalTextAlignCSS`
elizabetdev Jul 11, 2022
87f8e76
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev Jul 11, 2022
18f6555
More `logicalCSS`
elizabetdev Jul 11, 2022
74207d0
Removing `useIsWithinBreakpoints` and using a media query instead
elizabetdev Jul 12, 2022
9ad313b
Merge remote-tracking branch 'upstream/main' into emotion-image
cee-chen Jul 14, 2022
740563f
Update snapshots
cee-chen Jul 14, 2022
e2e5710
Improve wrapperProps test + fix aria-labels not being correctly passe…
cee-chen Jul 14, 2022
f882516
Fix missing logical CSS properties/values
cee-chen Jul 14, 2022
71ff6be
Refactor unnecessary JS logic
cee-chen Jul 14, 2022
0cf7eba
Refactor unnecessary JS logic + optimize hover animation
cee-chen Jul 14, 2022
b3e4825
Fix several props being incorrectly passed to underlying `img` tag as…
cee-chen Jul 14, 2022
e3533cf
Fix alignment issue between images and captions on mobile
cee-chen Jul 14, 2022
4445873
Fix floated image margins not collapsing w/ preceding paragraph on mo…
cee-chen Jul 14, 2022
825955c
Remove unnecessary CSS on figure wrappers
cee-chen Jul 14, 2022
f3c0db9
Clean up types
cee-chen Jul 15, 2022
2c11b45
Improve unit tests
cee-chen Jul 15, 2022
023d82c
Improve accessibilty documentation and screen reader experience for E…
cee-chen Jul 18, 2022
f580c80
[Misc] Convert image .js files to .tsx
cee-chen Jul 18, 2022
6f0a7d9
Fix axe lint complaint about fullscreen buttons with no text
cee-chen Jul 18, 2022
e48d39e
Copy tweaks
cee-chen Jul 18, 2022
223346f
Fix certain styles responding incorrectly when `isFullScreen` is true
cee-chen Jul 19, 2022
b8f6f67
[PR feedback] Include a chart/diagram example + alt text in demos
cee-chen Jul 19, 2022
04a1695
[Misc] Fix incorrect viewport unit being used for max-width
cee-chen Jul 19, 2022
55b8028
[cleanup] remove unnecesssary `isFullScreen` conditional
cee-chen Jul 19, 2022
c1b4c57
Adding accessibility improvements to changelog
constancecchen Jul 20, 2022
2e983ca
Add float fallbacks for browsers that don't yet support logical prope…
cee-chen Jul 20, 2022
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';

import { EuiImage, EuiText } from '../../../../src/components';
// @ts-ignore faker has no Typescript defs
import { fake } from 'faker';

export default () => (
Expand All @@ -10,9 +11,9 @@ export default () => (
float="right"
margin="l"
hasShadow
caption="Random nature image"
caption="A randomized image"
allowFullScreen
alt="Random nature image"
alt="" // Because the image is randomized, there is no meaningful alt text we can generate here.
src="https://picsum.photos/800/500"
/>
<p>{fake('{{lorem.paragraphs}}')}</p>
Expand All @@ -24,8 +25,8 @@ export default () => (
margin="l"
hasShadow
allowFullScreen
caption="Another random image"
alt="Random nature image"
caption="Another randomized image"
alt="" // Because the image is randomized, there is no meaningful alt text we can generate here.
src="https://picsum.photos/300/300"
/>
<p>{fake('{{lorem.paragraphs}}')}</p>
Expand Down
13 changes: 0 additions & 13 deletions src-docs/src/views/image/image.js

This file was deleted.

17 changes: 17 additions & 0 deletions src-docs/src/views/image/image.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import React from 'react';

import { EuiImage } from '../../../../src/components';

export default () => (
<EuiImage
size="l"
hasShadow
caption={
<p>
<em>Mastigias papua</em>, also known as spotted jelly
</p>
}
alt="Many small white-spotted pink jellyfish floating in a dark aquarium"
src="https://images.unsplash.com/photo-1650253618249-fb0d32d3865c?w=900&h=900&fit=crop&q=60"
/>
);
57 changes: 44 additions & 13 deletions src-docs/src/views/image/image_example.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,47 @@ import React, { Fragment } from 'react';

import { GuideSectionTypes } from '../../components';

import { EuiCode, EuiCallOut, EuiImage } from '../../../../src/components';
import {
EuiCode,
EuiCallOut,
EuiLink,
EuiImage,
} from '../../../../src/components';
EuiImage.__docgenInfo.props.src.required = true;

import imageConfig from './playground';

import Image from './image';
const imageSource = require('!!raw-loader!./image');
const imageSnippet = `<EuiImage
alt={description}
src={someSrc}
alt={alt}
src={src}
/>
`;

import ImageSizes from './image_size';
const imageSizesSource = require('!!raw-loader!./image_size');
const imageSizesSnippet = `<EuiImage
size="l"
alt={description}
src={someSrc}
alt={alt}
src={src}
/>
`;

import ImageZoom from './image_zoom';
const imageZoomSource = require('!!raw-loader!./image_zoom');
const imageZoomSnippet = `<EuiImage
allowFullScreen
alt={description}
src={someSrc}
alt={alt}
src={src}
/>
`;

import ImageFloat from './float';
const imageFloatSource = require('!!raw-loader!./float');
const imageFloatSnippet = `<EuiImage
alt={description}
src={someSrc}
alt={alt}
src={src}
float="left"
margin="l"
/>
Expand All @@ -54,10 +59,36 @@ export const ImageExample = {
},
],
text: (
<p>
Use <strong>EuiImage</strong> when you need to place a static image
into a page with an optional caption.
</p>
<>
<p>
Use <strong>EuiImage</strong> when you need to place a static image
into a page with an optional caption.
</p>
<EuiCallOut
iconType="accessibility"
title="Writing meaningful image alt text"
>
<p>
This page has several examples of alt text written to aid screen
Copy link
Contributor

Choose a reason for hiding this comment

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

I read through this description a bunch of times. What do you think about revising slightly to something like

This page has examples of alt text written to aid screen reader users, and examples of when not to include alt text. When an image provides key information, pass a descriptive string. When an image is decorative or repeats information, pass an empty "" string.

I was looking for a way to give short examples of when to, and when not to pass alt text. Feel free to modify or continue to riff on this language.

Copy link
Contributor

@cee-chen cee-chen Jul 18, 2022

Choose a reason for hiding this comment

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

When an image provides key information, pass a descriptive string.

This recommendation is tripping me up because there's a lot of nuance to "key information" that's difficult to convey in just 1 short sentence. I think it's better to defer to webAIM's article for this as it dives into a lot of the details of what kind of context and function to convey.

When an image is decorative or repeats information,

I don't think "repeats information" is sufficient here - I think we need the full if the image is adequately described by surrounding text sentence. For example in webAIM's image of Ellen Ochoa, Astronaut, the image itself doesn't repeat information, but the image is already described by the figcaption text and as such does not need repetitive alt text.

FWIW this is definitely a hobby horse for me and something I saw a lot of front end devs stumble on when we asked it as an interview question. I personally think it's worth highlighting 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

You make a good point about devs missing on this in interviews. Would you mind adding a sentence or two when a dev should include an alt text string? The block you have now sets the scene for alt text, and explains when to use an empty string really well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding a sentence or two when a dev should include an alt text string?

Hm, I thought this was implied in the copy that devs should include an alt text string whenever the image the image is not decorative or not already described by surrounding text. I can tweak the copy even more to clarify that.

The problem with going into more detail than that is that what's actually in the alt text is so contextual that it depends on what the page content and copy is about - we can't really be more prescriptive than that without writing an entire article, which doesn't fit in a callout. It makes more sense to link to WebAIM's article which does a much better job than we would of providing examples of what 'good' alt text is.

Copy link
Contributor

Choose a reason for hiding this comment

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

reader users, as well as several examples of when <em>not</em> to
include alt text. When an image is decorative, or if the image is
adequately described by surrounding text, it is better to pass an
empty <EuiCode>{'""'}</EuiCode> string instead.
</p>
<p>
When an image is not already sufficiently described, the alt text
passed should help non-visual users understand the purpose of the
image within the context of the overall page. See{' '}
<EuiLink
href="https://webaim.org/techniques/alttext/"
target="_blank"
>
WebAIM
</EuiLink>{' '}
for a more detailed guide to writing effective alt text.
</p>
</EuiCallOut>
</>
),
props: { EuiImage },
demo: <Image />,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,68 +2,74 @@ import React from 'react';

import { EuiImage, EuiSpacer } from '../../../../src/components';

const src =
'https://images.unsplash.com/photo-1477747219299-60f95c811fef?w=1000&h=1000&fit=crop&q=60';
const alt =
'A cozy breakfast scene. In the background is a plate of waffles and blueberries. In the middle ground is a glass of orange juice and a small cup of cream. In the foreground is a plate of Eggs Benedict with a side of salad and cherry tomatoes.';

export default () => (
<div>
<EuiImage
hasShadow
allowFullScreen
size={50}
caption="Custom size (50)"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
wrapperProps={{ className: 'eui-textLeft' }}
/>
<EuiSpacer />
<EuiImage
size="s"
hasShadow
allowFullScreen
caption="Small"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
<EuiSpacer />
<EuiImage
size="m"
hasShadow
allowFullScreen
caption="Medium"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
<EuiSpacer />
<EuiImage
size="l"
hasShadow
allowFullScreen
caption="Large"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
<EuiSpacer />
<EuiImage
size="xl"
hasShadow
allowFullScreen
caption="Extra large"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
<EuiSpacer />
<EuiImage
hasShadow
allowFullScreen
caption="Original"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
<EuiSpacer />
<EuiImage
hasShadow
allowFullScreen
size="fullWidth"
caption="Full width"
alt="Accessible image alt goes here"
src="https://source.unsplash.com/1000x1000/?Nature"
alt={alt}
src={src}
/>
</div>
);
33 changes: 0 additions & 33 deletions src-docs/src/views/image/image_zoom.js

This file was deleted.

44 changes: 44 additions & 0 deletions src-docs/src/views/image/image_zoom.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import React from 'react';

import {
EuiImage,
EuiFlexGroup,
EuiFlexItem,
EuiLink,
} from '../../../../src/components';

export default () => (
<EuiFlexGroup justifyContent="spaceEvenly">
<EuiFlexItem grow={false}>
<EuiImage
size="m"
hasShadow
allowFullScreen
caption="Albert Einstein, theoretical physicist"
alt="" // Because this image is sufficiently described by its caption, there is no need to repeat it via alt text
src="https://upload.wikimedia.org/wikipedia/commons/d/d3/Albert_Einstein_Head.jpg"
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiImage
allowFullScreen
caption={
<>
Browser usage on{' '}
<EuiLink
href="https://commons.wikimedia.org/wiki/File:Wikimedia_browser_share_pie_chart.png"
target="_blank"
>
Wikimedia (CC BY 3.0)
</EuiLink>
</>
}
alt="Pie chart describing browser usage on Wikimedia on October 2011. Internet Explorer occupies 34 percent, Firefox occupies 23 percent, Chrome occupies 20 percent, Safari occupies 11 percent, Opera occupies 5%, Android occupies 1.9 percent, and other browsers occupy 3.5 percent."
src="https://upload.wikimedia.org/wikipedia/commons/a/a2/Wikimedia_browser_share_pie_chart.png"
fullScreenIconColor="dark"
size={300}
style={{ padding: '20px 30px', background: 'white' }}
/>
</EuiFlexItem>
</EuiFlexGroup>
);
Loading