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

[EuiImage] Fix bug in small images not respecting the optional sizes when allowFullScreen is true #4207

Merged
merged 14 commits into from
Nov 4, 2020

Conversation

miukimiu
Copy link
Contributor

@miukimiu miukimiu commented Oct 30, 2020

Summary

This PR closes #4055.

When allowFullScreen was set to true the optional sizes were not respected in small images. This PR forces the small images to respect all the optional sizes.

Images Allow fullscreen@2x

How to test

This could be tested by adding this CodeSandbox Example into src-docs/src/views/image/image_size.js.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs
  • ~[ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@miukimiu miukimiu changed the title [EuiImage] Fix bug in small images not respecting the optional sizes when `` [EuiImage] Fix bug in small images not respecting the optional sizes when allowFullScreen is true Oct 30, 2020
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4207/

@miukimiu miukimiu requested a review from cchaos October 30, 2020 18:57
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Thanks! This fixes the issue.

The other oddity I noticed is the placement sometimes of the fullScreen and close icons. Because they're being positioned against the entire figure (containing the caption), it doesn't always overlap the image. Perhaps we can just add position:relative to the button and then move the close icon to be positioned against the window instead of the image.

image
image

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4207/

@miukimiu miukimiu requested a review from cchaos November 3, 2020 17:22
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4207/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

👍 LGTM. The only other tweak you might want to consider is that the overlay mask in Amsterdam is actually dark and not light so it's hard to see the close button.
Screen Shot 2020-11-04 at 13 36 15 PM

@miukimiu
Copy link
Contributor Author

miukimiu commented Nov 4, 2020

jenkins test this

1 similar comment
@miukimiu
Copy link
Contributor Author

miukimiu commented Nov 4, 2020

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4207/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiImage] Using allowFullScreen on an image with a data URI source doesn't respect sizing
3 participants