-
Notifications
You must be signed in to change notification settings - Fork 842
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
Changes from 45 commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
efd2852
Initial conversion
elizabetdev 3a1244d
Fixing close button color
elizabetdev f558663
Removing unused sass files
elizabetdev 756b90c
Adding CL
elizabetdev 1ae8892
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev 2247ac9
Better conditions and styles
elizabetdev ecb63f5
Adding _imageMargins style function
elizabetdev e062665
WIP
elizabetdev 7f6c8e5
Improving `_imageWidth`
elizabetdev 016bd62
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev c801e40
Splitting component in `EuiImage` and `EuiImageWrapper`.
elizabetdev cba8d5a
Improving `EuiText` image styles
elizabetdev e0e075b
Better styles
elizabetdev 4cb1893
Adding `shouldRenderCustomStyles`
elizabetdev 50ef8b2
Renaming styles
elizabetdev 2268d5a
splitting in more components
elizabetdev bddf55f
Better components split
elizabetdev d85a683
Fixing caption hover styles
elizabetdev c4de638
Omit onClick from EuiImageWrapperProps and EuiImageFullScreenWrapperP…
chandlerprall 0bf23ee
Reorganizing imports and types
elizabetdev f7cff69
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev 3dcf262
Better styles
elizabetdev 4d9b546
Adding `commonImgProps` and `commonWrapperProps`
elizabetdev 51fa9e6
CL
elizabetdev e52dad8
CL again
elizabetdev 62dbd45
Adding `euiImageFullScreenWrapper` className
elizabetdev 2ee3c50
Adding `Breaking changes` CL entry
elizabetdev 6a6cf5c
Adding `logicalTextAlignCSS`
elizabetdev 87f8e76
Merge remote-tracking branch 'upstream/main' into emotion-image
elizabetdev 18f6555
More `logicalCSS`
elizabetdev 74207d0
Removing `useIsWithinBreakpoints` and using a media query instead
elizabetdev 9ad313b
Merge remote-tracking branch 'upstream/main' into emotion-image
cee-chen 740563f
Update snapshots
cee-chen e2e5710
Improve wrapperProps test + fix aria-labels not being correctly passe…
cee-chen f882516
Fix missing logical CSS properties/values
cee-chen 71ff6be
Refactor unnecessary JS logic
cee-chen 0cf7eba
Refactor unnecessary JS logic + optimize hover animation
cee-chen b3e4825
Fix several props being incorrectly passed to underlying `img` tag as…
cee-chen e3533cf
Fix alignment issue between images and captions on mobile
cee-chen 4445873
Fix floated image margins not collapsing w/ preceding paragraph on mo…
cee-chen 825955c
Remove unnecessary CSS on figure wrappers
cee-chen f3c0db9
Clean up types
cee-chen 2c11b45
Improve unit tests
cee-chen 023d82c
Improve accessibilty documentation and screen reader experience for E…
cee-chen f580c80
[Misc] Convert image .js files to .tsx
cee-chen 6f0a7d9
Fix axe lint complaint about fullscreen buttons with no text
cee-chen e48d39e
Copy tweaks
cee-chen 223346f
Fix certain styles responding incorrectly when `isFullScreen` is true
cee-chen b8f6f67
[PR feedback] Include a chart/diagram example + alt text in demos
cee-chen 04a1695
[Misc] Fix incorrect viewport unit being used for max-width
cee-chen 55b8028
[cleanup] remove unnecesssary `isFullScreen` conditional
cee-chen c1b4c57
Adding accessibility improvements to changelog
constancecchen 2e983ca
Add float fallbacks for browsers that don't yet support logical prope…
cee-chen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
/> | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import React from 'react'; | ||
|
||
import { | ||
EuiImage, | ||
EuiFlexGroup, | ||
EuiFlexItem, | ||
} from '../../../../src/components'; | ||
|
||
export default () => ( | ||
<EuiFlexGroup> | ||
<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 | ||
size="m" | ||
hasShadow | ||
allowFullScreen | ||
caption="Marie Curie, physicist and chemist" | ||
alt="" // Because this image is sufficiently described by its caption, there is no need to repeat it via alt text | ||
fullScreenIconColor="dark" | ||
src="https://upload.wikimedia.org/wikipedia/commons/a/a3/Marie_Sklodowska%2C_%C3%A9tudiante%2C_en_1895.jpg" | ||
/> | ||
</EuiFlexItem> | ||
</EuiFlexGroup> | ||
); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 ofEllen Ochoa, Astronaut
, the image itself doesn't repeat information, but the image is already described by thefigcaption
text and as such does not need repetitivealt
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e48d39e