allow EuiImage to set custom width #3012

anishagg17 · 2020-03-09T11:27:04Z

Summary

Fixes : #1554

allow EuiImage to set custom width by allowing size to accept number

Checklist

~~- [ ] Check against all themes for compatibility in both light and dark modes~~
- [ ] Checked in mobile

Checked in IE11 and Firefox
- [ ] Props have proper autodocs
Added documentation 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

kibanamachine · 2020-03-09T11:27:05Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

thompsongl

Thanks, @anishagg17

Can you update the PR summary to include a more full description? Like, what part of the issue this PR addresses, how it was addressed, and what is still left to do?

Also, please strikethrough (~~) any checklist items that don't apply so we have an accurate picture of work still left to be done.

thompsongl

Thanks for the description and summary updates

This doesn't appear to match the description or solve the issue requirements, though. Specifically:

the size prop essentially means "width or height should be set to this, whichever is larger" in any cases when it is not set to original or fullWidth

allow EuiImage to set custom width by allowing size to accept number

With these changes, only height is directly set when giving the size prop a numeric value. A 2:1 horizontally oriented image, for instance, would be 100px wide when size={50}. This component would need to do an aspect ratio calculation to accomplish the requirement of "width or height should be set to this, whichever is larger" (or perhaps just also set max-width to the given size number)

@snide Am I reading your comments correctly?

anishagg17 · 2020-03-14T12:06:44Z

I will update it soon

anishagg17 · 2020-03-14T14:42:54Z

I have updated the functionality

thompsongl · 2020-03-16T16:01:41Z

@anishagg17 I'm still a bit confused on the intention of this PR.
The last commit message says that height should be a consideration (greater of height or width gets size ,lesser is set to auto), but the description of the PR still only accounts for width.
And, functionally, only width is currently impacted (50px width regardless of aspect ratio):

If you intend for this PR to only impact width, we can work with that. If your latest commit intended to restrict height based on aspect ratio, the functionality needs to be revisited.

anishagg17 · 2020-03-16T16:14:23Z

Let me recheck for it

anishagg17 · 2020-03-16T17:58:31Z

@thompsongl can you please recheck this now

thompsongl

Thanks, @anishagg17, this does restrict height now also.

Like I mentioned in an earlier comment, this can be done without any JS if we were to "perhaps just also set max-width to the given size number". Rather than use timeouts or ratio calculations, we could use the customStyle state to set minWidth and maxWdith to size when size is a number. CSS will correctly size the image with the same restrictions.

src/components/image/_image.scss

anishagg17 · 2020-03-17T15:32:03Z

Rather than use timeouts or ratio calculations, we could use the customStyle state to set minWidth and maxWdith to size when size is a number. CSS will correctly size the image with the same restrictions.

Should I update according to this??

cchaos · 2020-03-17T22:12:16Z

Jenkins, test this

kibanamachine · 2020-03-17T22:32:16Z

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

thompsongl · 2020-03-17T22:55:22Z

Should I update according to this??

Yes, please. A style-only solution is preferable here to using React lifecycle methods and JS intervals to check DOM sizing attributes.

anishagg17 · 2020-03-18T10:17:22Z

I tried your solution but it does not maintain the aspect ratio 100x800 image would turn to 50x400

also when i apply same to height then the output is 50x50 it should be 50x40

anishagg17 · 2020-03-18T10:34:01Z

now i have used

 customStyle = {
        maxWidth: size,
        maxHeight: size,
        minWidth: 0,
        minHeight: 0,
        width: 'auto',
        height: 'auto',
      };

this fixes it correctly

cchaos · 2020-03-18T13:10:37Z

@anishagg17 This one got kind of complicated and the docs needed good explanations so I pushed you a PR. anishagg17#3 As mentioned in the summary it:

Moved props docs to comments in component file

Accounting for style prop is passed by consumer

Removed extraneous style properties

Updating docs size section

- Moved props docs to comments in component file - Accounting for `style` prop is passed by consumer - Removed extraneous style properties - Updating docs size section

anishagg17 · 2020-03-18T13:47:32Z

@cchaos

maxWidth: size,
maxHeight: size,
width: 'auto',
height: 'auto',

Last 2 properties are required to maintain the aspect ratio otherwise a size x size image is rendered

cchaos · 2020-03-18T14:11:34Z

Ahh it only happens when the height is greater than the width. This actually brings up a greater question... Should we be increasing the size of the images to fit the "size" given?

@snide, Just curious on your thoughts here....

Currently, all the enum sizes (s, m, l, etc...) will force the image to grow to fit that specified width. However, in the current configuration when a number is provided, this is maximum width/height. So if they supply a smaller image than the number provided, it does not grow to fit.

I can't really see a benefit of making the image grow to fit the size if the image isn't already atleast that size. This will cause blurry images.

cchaos · 2020-03-18T22:30:52Z

Jenkins test this

kibanamachine · 2020-03-18T22:50:51Z

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

snide · 2020-03-18T23:03:11Z

I can't really see a benefit of making the image grow to fit the size if the image isn't already atleast that size. This will cause blurry images.

I agree.

cchaos

I've thought about it some more and I think we should just keep the behavior as it is where the sized (s, m, l, etc) images would grow to fit the determined size, but the custom values behave as a maximum and wont grow the image.

The only way I could find to make the latter actually grow the image proportionally is to use object-fill but then it doesn't work with the shadow we have on the figure and just gets very messy. So for now, I think this works. I just have some text changes now based on all that.

CHANGELOG.md

src-docs/src/views/image/image_example.js

src/components/image/image.tsx

cchaos · 2020-03-18T23:06:09Z

I agree.

Haha, thanks @snide. I think we'll leave it for now and circle back but at least the custom sizing won't increase the image size.

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

cchaos · 2020-03-19T15:33:50Z

Jenkins, test this

kibanamachine · 2020-03-19T15:52:59Z

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

cchaos · 2020-03-19T16:21:43Z

There's a prettier error most likely due to the accepting of changes in Github.

11:40:31 /app/src-docs/src/views/image/image_example.js
11:40:31   109:75  error  Insert `⏎·········`  prettier/prettier
11:40:31 
11:40:31 ✖ 1 problem (1 error, 0 warnings)
11:40:31   1 error and 0 warnings potentially fixable with the `--fix` option.

cchaos · 2020-03-19T17:32:48Z

Jenkins, test this

kibanamachine · 2020-03-19T17:51:59Z

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

cchaos

Looks good to me. Thanks!

allow EuiImage to set custom width

fd10148

Update CHANGELOG.md

3b1522e

thompsongl reviewed Mar 9, 2020

View reviewed changes

anishagg17 added 2 commits March 11, 2020 02:56

Merge branch 'master' into test2

21bb0ef

Merge branch 'master' into test2

119f8b9

thompsongl reviewed Mar 12, 2020

View reviewed changes

Merge branch 'master' into test2

f5c334c

greater of height or width gets size ,lesser is set to auto

f4247c7

checked for both heaight and width

ba123dd

anishagg17 added 2 commits March 16, 2020 23:28

Merge branch 'master' into test2

d4bf580

Merge branch 'master' into test2

01cedb4

thompsongl reviewed Mar 17, 2020

View reviewed changes

src/components/image/_image.scss Outdated Show resolved Hide resolved

src/components/image/_image.scss Outdated Show resolved Hide resolved

anishagg17 and others added 3 commits March 17, 2020 21:38

removed uneccessary className

33cdb73

Merge branch 'test2' of https://github.com/anishagg17/eui into test2

7e60550

Merge branch 'master' into test2

bfa8665

used maxWidth,maxHeight

a7a510f

anishagg17 added 2 commits March 18, 2020 16:06

removed class

3d66262

Merge branch 'test2' of https://github.com/anishagg17/eui into test2

d54ab10

added test for custom size

5dd124a

cchaos and others added 2 commits March 18, 2020 19:06

Pushing some extra help (#3)

f5a263c

- Moved props docs to comments in component file - Accounting for `style` prop is passed by consumer - Removed extraneous style properties - Updating docs size section

maintain the aspect ratio

f86d126

cchaos suggested changes Mar 18, 2020

View reviewed changes

anishagg17 and others added 7 commits March 19, 2020 12:05

Update src/components/image/image.tsx

fbb0bd7

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update CHANGELOG.md

174ed01

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update src-docs/src/views/image/image_example.js

d8f6fcc

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update src-docs/src/views/image/image_example.js

84b2c92

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

Update src/components/image/image.tsx

f8a2521

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

updated snap

280a1f5

Merge branch 'master' into test2

a1d309f

fixed prettier

0c565aa

cchaos approved these changes Mar 19, 2020

View reviewed changes

cchaos merged commit bedc400 into elastic:master Mar 19, 2020

anishagg17 deleted the test2 branch March 19, 2020 19:09

chandlerprall mentioned this pull request Mar 31, 2020

Upgrade to EUI v22.1.1 elastic/kibana#62065

Closed

chandlerprall mentioned this pull request Apr 8, 2020

Upgrade to EUI v22.3.0 elastic/kibana#62963

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow EuiImage to set custom width #3012

allow EuiImage to set custom width #3012

anishagg17 commented Mar 9, 2020 •

edited

Loading

kibanamachine commented Mar 9, 2020

thompsongl left a comment

thompsongl left a comment •

edited

Loading

anishagg17 commented Mar 14, 2020

anishagg17 commented Mar 14, 2020

thompsongl commented Mar 16, 2020

anishagg17 commented Mar 16, 2020

anishagg17 commented Mar 16, 2020

thompsongl left a comment

anishagg17 commented Mar 17, 2020

cchaos commented Mar 17, 2020

kibanamachine commented Mar 17, 2020

thompsongl commented Mar 17, 2020

anishagg17 commented Mar 18, 2020 •

edited

Loading

anishagg17 commented Mar 18, 2020

cchaos commented Mar 18, 2020

anishagg17 commented Mar 18, 2020

cchaos commented Mar 18, 2020

cchaos commented Mar 18, 2020

kibanamachine commented Mar 18, 2020

snide commented Mar 18, 2020

cchaos left a comment

cchaos commented Mar 18, 2020

cchaos commented Mar 19, 2020

kibanamachine commented Mar 19, 2020

cchaos commented Mar 19, 2020

cchaos commented Mar 19, 2020

kibanamachine commented Mar 19, 2020

cchaos left a comment

allow EuiImage to set custom width #3012

allow EuiImage to set custom width #3012

Conversation

anishagg17 commented Mar 9, 2020 • edited Loading

Summary

Checklist

kibanamachine commented Mar 9, 2020

thompsongl left a comment

Choose a reason for hiding this comment

thompsongl left a comment • edited Loading

Choose a reason for hiding this comment

anishagg17 commented Mar 14, 2020

anishagg17 commented Mar 14, 2020

thompsongl commented Mar 16, 2020

anishagg17 commented Mar 16, 2020

anishagg17 commented Mar 16, 2020

thompsongl left a comment

Choose a reason for hiding this comment

anishagg17 commented Mar 17, 2020

cchaos commented Mar 17, 2020

kibanamachine commented Mar 17, 2020

thompsongl commented Mar 17, 2020

anishagg17 commented Mar 18, 2020 • edited Loading

anishagg17 commented Mar 18, 2020

cchaos commented Mar 18, 2020

anishagg17 commented Mar 18, 2020

cchaos commented Mar 18, 2020

cchaos commented Mar 18, 2020

kibanamachine commented Mar 18, 2020

snide commented Mar 18, 2020

cchaos left a comment

Choose a reason for hiding this comment

cchaos commented Mar 18, 2020

cchaos commented Mar 19, 2020

kibanamachine commented Mar 19, 2020

cchaos commented Mar 19, 2020

cchaos commented Mar 19, 2020

kibanamachine commented Mar 19, 2020

cchaos left a comment

Choose a reason for hiding this comment

anishagg17 commented Mar 9, 2020 •

edited

Loading

thompsongl left a comment •

edited

Loading

anishagg17 commented Mar 18, 2020 •

edited

Loading