[Emotion] Convert EuiImage
#5969
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
elizabetdev
force-pushed
the
emotion-image
branch
from
86e96f2 to
cba8d5a
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
There was a problem hiding this comment.
@constancecchen LMK if you want to discuss any of the comments I made.
| @@ -10,9 +10,9 @@ export default () => ( | |||
| float="right" | |||
There was a problem hiding this comment.
I agree a randomized image makes more sense with an alt="" declaration. The image feels like a placeholder that has a high likelihood of not being relevant to the content.
I also noticed the <figure> has an aria-label attribute that it doesn't need. We should remove this because there's a native semantic relationship between <figure> and its child <figcaption> that we don't want to disrupt.
There was a problem hiding this comment.
I also noticed the
<figure>has an aria-label attribute that it doesn't need. We should remove this because there's a native semantic relationship between<figure>and its child<figcaption>that we don't want to disrupt.
I went back and forth on this as well but it turns out figure does actually need aria-label for Chrome+VO 🤪
https://www.scottohara.me/blog/2019/01/21/how-do-you-figure.html#chrome
Admittedly Chrome+VO is not a super common combo compared to Safari+VO, but honestly if Scott O'Hara recommends it I'm willing to follow it, I think?... 🤷♀️ Although admittedly 2019 is a whole 3 years ago at this point.
As an aside, this is where us having an official screen reader support matrix would come in handy!
There was a problem hiding this comment.
It seems Chrome hasn't patched this oversight yet in 2022. Agreed, let's keep the aria-label as a required foothold.
src-docs/src/views/image/image.js
Outdated
| caption="Random nature image" | ||
| alt="Random nature image" | ||
| src="https://picsum.photos/300/300" | ||
| caption={ |
There was a problem hiding this comment.
👍 This is a solid example of using alt text to describe the large number of jellyfish in the frame, and the caption to provide specific, technical information.
| title="Writing meaningful image alt text" | ||
| > | ||
| <p> | ||
| This page has several examples of alt text written to aid screen |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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.'; |
There was a problem hiding this comment.
VO reads this alt text out perfectly in Safari. I vaguely remember one of the big three only storing ~85 characters in an alt text buffer. I'm not recommending making any changes here, but let's keep it in the back of our minds that we may want to add a character count recommendation to alt text.
There was a problem hiding this comment.
Looks like the most recent tests on this indicate no character limit:
- https://yatil.net/blog/there-is-no-character-limit-for-alt-text
- Alt Text Character Limit w3c/wcag#2126
This is a super interesting point tho, particularly in the comments about alt text not being skippable and it being best to keep it short. Honestly it brings to mind the convos/topics I've read about accessible alt text for webcomics:
- https://axesslab.com/accessible-comics/ (I LOVE the second example/video here and think this is a great case for why full sentences should be acceptable within alt text)
- https://lastcallmedia.com/blog/accessible-comics
- https://spinweaveandcut.com/blind-accessible-comics/
And even things like accessible memes - memes are even less likely to be broken up into separate panels/images, but it feels like you'd miss a lot of context there without full sentences.
I admit that very detailed description of images would make sense for, e.g. social media or a photographer's image gallery, but less so for the use cases that Kibana will typically face. I wonder if it would make more sense for us to include an image of a graph or other statistic-based diagram instead of an Instagram-esque photo. What do you think? 🙌
There was a problem hiding this comment.
These are good examples of long, meaningful alt text, thank you for linking to them.
I agree there's nuance around what alt text should say and how long it should be. I see roughly four broad cases:
- Decorative image - Offers no additional context to the page or is a visual device only. Empty
alt=""here. - Supporting information image - Thinking an illustration to highlight a key detail or coaching point. This would need a shorter alt text string that reinforces the text narrative in a related, but not repeated way.
- Rich illustrations, memes, comics - These are pure visual information. Having a long alt text string like the examples you linked above are excellent.
- Rich chart / data visualizations - Charts and visuals should have alt text that identify the key information (title, time range, data point) but any summary information like "Alerts increased 5% over this time range" should be surfaced in a
<figcaption>as a child element to<figure>. In this case, the summary text would be beneficial to all users and should be incorporated as a design and UX feature.
It might be worth adding a stats-based diagram to document use case 4, and I think having the Insta-esque photo is good too. There's definitely editorial use cases, and having it documented means we serve a wider audience.
There was a problem hiding this comment.
Thanks Trevor! I added a chart example in b8f6f67:
Let me know what you think! I think this rounds out our alt examples fairly well. The resources I used for writing the alt text for charts were:
| > | ||
| {children} | ||
| </EuiImageButton> | ||
| <p id={describedById} hidden> |
There was a problem hiding this comment.
This is a pretty useful pattern for building accessible labels that aren't addenda to visible labels. Confirmed it's valid and shouldn't have any surprises reading this article by Steve Faulker at Paciello Group: https://www.tpgi.com/short-note-on-aria-labelledby-and-aria-describedby/
There was a problem hiding this comment.
I actually copied the paradigm from our EuiDataGrid SR text! Definitely agreed it's cool and comes in handy for describedby.
FYI though @1Copenut, I had to remove aria-describedby because it was failing axe linting 😞 You might have to re-test on latest staging deploy. Sorry for the back and forth on this!
There was a problem hiding this comment.
UPDATE:
I'm on the fence about the aria-label being applied to the svg[role="img"] than the <button> itself, but if it tests well, I won't recommend changing it.
That's unexpected, but we'll work with it.
There was a problem hiding this comment.
The problem with applying aria-label to the button is it completely overrides the nested img tag's alt text.
I actually ended up revisiting the aria-label on the icon (yet again) in 6f0a7d9 because the icon has a different absolute position in fullscreen mode and can't be nested in the figure because the figure has a transform on it. So the instructions now live as EuiScreenReaderOnly text 💀
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
- we can't use `aria-describedby` unfortunately because that doesn't qualify as a button name - so instead I'm using SR-only text to describe the fullscreen UX (+ with bonus short circuiting in fullscreen mode for images with long alt text) - hasAlt and an em dash gets us somewhat close to previous describedby behavior (adding a pause between img alt and the button behavior description) - I'm also removing `allowFullScreen` from being passed as a prop to image_button - this button can safely assume it's only used/called when fullscreen mode is enabled
- customSize styles should not be lost when going into fullscreen mode; the fullscreen image should simply not have said styles - isFullScreen is always true when the fullscreen wrapper is being used, and vice versa for the non-fullscreen wrapper
+ fix link color styling in fullscreen mode
|
After another round of docs tweaks and bug fixes, I think this is good to go! @1Copenut I pushed up another screen reader pass at the fullscreen SR instructions, this time using Let me know if you see any issues or blockers, otherwise I'd love to merge this in soon! |
- open/close icons & their styles are declared separately, so no need for conditional logic
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
This conditional order switch to put the button control announcement first on fullscreen was really nice. I was able to understand the instructions and listen to the alt text if I wanted, or back out to the regular screen view without the whole string read back. Thank you for adding a pie chart graphic. This will be a great example as we push the envelope of accessible charting. Accessibility is a go! 👍 |
|
@constancecchen I noticed that the image with floats are not floating: https://eui.elastic.co/pr_5969/#/display/image#float-images-within-text. |
…rties - Chrome, Opera, Edge appear to be the primary culprits (without a flag enabled)
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
Thanks so much for catching that @miukimiu!! TIL not all browsers support logical properties with I was testing primarily on FF and Safari and totally missed Chrome 🤦 I've added a fallback value that I can confirm makes it so it works on Chrome but also that the |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5969/ |
|
LGTM @constancecchen! I just found one more issue. Before the conversion to Emotion for small screens and if the image had any float, the wrapper
|
|
TBH I think that was previously incorrect CSS that is now fixed. The |
|
It makes sense! So let's merge it! 🎉 |
Summary
This PR converts the EuiImage to Emotion and adds a new prop called
wrapperProps. UpdatedEuiImage's full screen mode to use thefullScreenExiticon.Split in multiple components
A good reason to split the component is that the
classNamewas being applied to the wrapping<figure />element andstyle,classNameand...restto the<img />element. This was a little bit confusing for consumers.For this reason, I split the
EuiImageinto multiple components:style,classNameand...restprops are applied to this component.Internal
wrapperProps.wrapperProps.New
wrapperPropsA new prop called
wrapperPropswas added. This ensures consumers can also customize the wrapperfigurearound the image.Preventing EuiText images from always getting 100%
When a
<img />was inside aEuiTextit would get a 100% of the parent width. This was wrong and was conflicting withEuiImages inside aEuiText.So I fixed the
EuiText.imgtomax-width: 100%;. IMO the images should keep their original size when added inside aEuiText. Let's say I had an image with60pxof width. Why would this image grow to 100%?Design updates
The image focus outline is now using the browser default and the
EuiImage's full-screen mode is now using afullScreenExiticon instead of across. This makes the full-screen mode get consistent with other components.Breaking change
A
classNamepassed to theEuiImagewould go to the wrapperfigure. With the new changes, it goes directly to theimgelement. So it is required to update all the instances that use:<EuiImage className="myClassName" />to<EuiImage wrapperProps={{className: "myClassName"}} />.euiImageCSS overrides with.euiImageWrapper.euiImage-isFullScreenwith.euiImageFullScreenWrapper.These are all the usages I found:
Checklist