Image component #262

snide · 2018-01-04T04:12:23Z

Work in progress on an Image component. Need to discuss with @cchaos and figure out some stuff around sizing.

cc @alexfrancoeur @nreese. This will give you what you need for the expandable screenshots in the homepage v2.

Questions for review

Should hasShadow be on by default?
Should allowFullScreen be on by default?
Should a default size be set?

cchaos · 2018-01-04T16:15:25Z

Putting here mainly as a reminder:

We should be sure to require the alt attribute on img tags.

nreese

This will be great for the home page.

What happened to the dungeons and dragons image?

nreese · 2018-01-04T21:17:31Z

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

+          </p>
+          <ul>
+            <li>
+              <EuiCode>size</EuiCode> accepts <EuiCode>s / m / l / original / fullWidth</EuiCode>.


Missing xl option

nreese · 2018-01-04T21:18:34Z

src/components/image/image.js

+    super(props);
+
+    this.state = {
+      isImageFullscreen: false,


Should isImageFullscreen be camel-cased and be isImageFullScreen?

nreese · 2018-01-04T21:20:54Z

src/components/image/image.js

+    this.onKeyDown = this.onKeyDown.bind(this);
+  }
+
+  onKeyDown = event => {


This is not passed as a prop to anything so pressing the esc key does not close the full screen image

Ha. Knew I forgot something ;-)

snide · 2018-01-04T22:22:23Z

@nreese Feedback addressed and added an ESC to close mechanism.

nreese

lgtm

cchaos

No shadow should not be default -- Should shadow only show if it's clickable?
Allow full screen is harder because it should really only happen if the image is displayed much smaller than the original. Not sure what the typical implementation will be.
I think the default size should be original.

Do we want to address the hover states (icon overlay) in this PR or later?

cchaos · 2018-01-04T22:42:24Z

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

+          <ul>
+            <li>
+              <EuiCode>size</EuiCode> accepts <EuiCode>s / m / l / xl / original / fullWidth</EuiCode>.
+              The later will set the figure to stretch to 100% of its container.


sp: "later" -> "latter"

cchaos · 2018-01-04T22:43:25Z

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

+            </li>
+            <li>
+              <EuiCode>caption</EuiCode> will provide a caption to the image.
+            </li>


title is a prop as well

cchaos · 2018-01-04T22:43:46Z

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

+      text: (
+        <p>
+          Images can be sized by passing the <EuiCode>size</EuiCode> prop a value
+          of <EuiCode>s / m / l / original / fullWidth</EuiCode>.


add 'xl' to this list as well

snide · 2018-01-05T20:57:21Z

OK. Feedback addressed.

Gonna keep the shadow as on by default since it matches our other elements (like form elements) which have a similar small shadow. Ability is still there to turn it off.
allowFullScreen is set to off by default. Updated the doc examples.
Default size set to original.

I also went ahead and added a small expand icon. This requires a new prop fullScreenIconColor though because the images might be dark or light and need a different color hover icon. So added that as well.

cchaos

Looks good, except for one inconsistency with fullScreenIconColor.

cchaos · 2018-01-05T21:29:56Z

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

+        allowFullScreen
+        caption="I am a starship. Zooom!"
+        title="Click me to see in full screen."
+        fullScreenIconColor="default"


Should this be fullScreenIconColor="dark" not default?

cchaos · 2018-01-05T21:38:13Z

Oh and I just saw two more things, the second one we can either address now or when we do more in-depth documentation.

The title attribute should actually be alt
The title/alts of the image examples aren't correct alt descriptions. They should be describing the image not describing the interaction. I only mention this since we want to be sure that even our examples are fully correct so that someone who may be copy and pasting these directly from the examples will know what needs to be replaced, else they may just keep that title as is.

snide · 2018-01-05T21:53:42Z

Good spots. Flipped the alts in, adjusted their text and fixed that default/dark issue. Gonna merge.

alexfrancoeur · 2018-01-08T17:23:23Z

@snide woah! this is a fantastic surprise, awesome turnaround guys. @nreese is this something we can use in 6.2 or should I add to the enhancement issue I'll be creating for Kibana Home?

nreese · 2018-01-08T17:28:25Z

@alexfrancoeur Its already in 6.2 and being used on the home page

snide added feature request prototype labels Jan 4, 2018

snide mentioned this pull request Jan 4, 2018

Near term EUI roadmap #108

Closed

51 tasks

snide requested review from cchaos and nreese January 4, 2018 20:58

snide removed the work in progress label Jan 4, 2018

snide added 8 commits January 4, 2018 13:05

eui image component

7e5abcb

snapshots

6f16dfa

allow fullscreen

f7fd8a4

better docs, starting place for sizing

d2ff624

image titles

3e7745b

apply caroline's size suggestions, swap out cooler image

881db03

make test pass

2a8a1bb

make size required

9ef0a01

snide force-pushed the feature/image branch from c9f4e47 to 9ef0a01 Compare January 4, 2018 21:05

nreese reviewed Jan 4, 2018

View reviewed changes

add focustrap and ESC to close

06984d2

nreese approved these changes Jan 4, 2018

View reviewed changes

changelog

806696d

cchaos reviewed Jan 4, 2018

View reviewed changes

address feedback, improve docs

0440b38

cchaos suggested changes Jan 5, 2018

View reviewed changes

snide added 2 commits January 5, 2018 13:49

address feedback

e136191

adjust test

c2bf6e8

cchaos approved these changes Jan 5, 2018

View reviewed changes

snide merged commit 9ec3e40 into elastic:master Jan 5, 2018

snide deleted the feature/image branch January 5, 2018 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image component #262

Image component #262

snide commented Jan 4, 2018 •

edited

Loading

cchaos commented Jan 4, 2018 •

edited

Loading

nreese left a comment

nreese Jan 4, 2018

nreese Jan 4, 2018

nreese Jan 4, 2018

snide Jan 4, 2018

snide commented Jan 4, 2018

nreese left a comment

cchaos left a comment •

edited

Loading

cchaos Jan 4, 2018

cchaos Jan 4, 2018

snide Jan 5, 2018

cchaos Jan 4, 2018

snide commented Jan 5, 2018

cchaos left a comment

cchaos Jan 5, 2018

cchaos commented Jan 5, 2018

snide commented Jan 5, 2018

alexfrancoeur commented Jan 8, 2018

nreese commented Jan 8, 2018 •

edited

Loading

Image component #262

Image component #262

Conversation

snide commented Jan 4, 2018 • edited Loading

Questions for review

cchaos commented Jan 4, 2018 • edited Loading

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide commented Jan 4, 2018

nreese left a comment

Choose a reason for hiding this comment

cchaos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snide commented Jan 5, 2018

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Jan 5, 2018

snide commented Jan 5, 2018

alexfrancoeur commented Jan 8, 2018

nreese commented Jan 8, 2018 • edited Loading

snide commented Jan 4, 2018 •

edited

Loading

cchaos commented Jan 4, 2018 •

edited

Loading

cchaos left a comment •

edited

Loading

nreese commented Jan 8, 2018 •

edited

Loading