Custom space avatar images #45148

friol · 2019-09-09T15:11:32Z

Summary

As asked in Issue #43528, this PR introduces the possibility to add custom avatar images for spaces.

Resolves #43528

elasticmachine · 2019-09-09T15:11:34Z

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?

elasticmachine · 2019-09-09T15:11:34Z

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?

legrego · 2019-09-09T15:42:05Z

Hey @friol, thanks for this PR! I'll review this over the next day or so, and respond with any feedback.

elasticmachine · 2019-09-09T15:42:42Z

Pinging @elastic/kibana-security

legrego

Thanks again for taking the time to open this PR. What you have here is a great start -- it seems to be working for me as expected, with the exception of large images, which hit up against the server.maxPayloadBytes property. In practice, this won't be an issue because of other recommendations I've proposed below.

To update the Kibana index mapping to accept the new avatarImage / imageUrl field, you'll just need to update the Spaces's mappings.json file to include the new property. Kibana will automatically adjust the index template, and migrate the existing index to use the updated mappings. No need to open a different PR against Elasticsearch!

x-pack/legacy/plugins/spaces/common/space_attributes.ts

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx

x-pack/legacy/plugins/spaces/common/model/space.ts

x-pack/legacy/plugins/spaces/server/lib/space_schema.ts

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx

ryankeairns · 2019-09-10T13:36:08Z

Getting close, thanks for making changes!

It looks like you'll need to either truncate the image file name or make it wrap... there is an EUI mixin for this - @include euiTextTruncate;

Also, the background color needs to be set to either white or transparent when an image is in place. Without having tested this out, I presume transparent is the more expected setting.

Text overflowing container; Disabled background color showing through an image with transparency

friol · 2019-09-10T14:50:44Z

@ryankeairns : the filename wrapping should be fixed.

About transparent images, I've done some tests and with a transparent svg, the background color selected with the picker is shown.
I would personally leave this behaviour untouched, also because it should be embedded in the @elastic/eui library (another repository).

ryankeairns · 2019-09-10T18:02:13Z

@friol If we decide to keep the background color, then we should allow the user to change it. In other words, do not disable the Color input when an image is present.

friol · 2019-09-10T19:27:14Z

@friol If we decide to keep the background color, then we should allow the user to change it. In other words, do not disable the Color input when an image is present.

@ryankeairns : I feel this is a corner case. The user is still able to remove the image, change the background color, and re-upload the image. But if this is necessary, I can do it (and the colour picker will be always enabled, with or without an avatar image).

ryankeairns · 2019-09-10T20:00:20Z

Thanks, and yes we would consider it necessary.

Forcing a user to delete and re-upload their image is an undesirable flow that we should avoid, and It seems simple enough to now keep the color picker always enabled. I realize we first suggested it be disabled, but that was before realizing the color would still appear behind images with transparency.

…ent svgs

friol · 2019-09-10T21:39:54Z

Thanks, and yes we would consider it necessary.

Forcing a user to delete and re-upload their image is an undesirable flow that we should avoid, and It seems simple enough to now keep the color picker always enabled. I realize we first suggested it be disabled, but that was before realizing the color would still appear behind images with transparency.

@ryankeairns : Done!

legrego · 2019-09-11T12:39:12Z

@friol @ryankeairns do you think there's any value in persisting the image filename? IMO, storing the image alone should be sufficient.

I realize we display the filename when editing an existing space which contains a custom image, so we would need to figure out what to display there for that scenario, but it doesn't seem to add a whole lot of value to me. I also think it's awkward to ask API consumers to specify this, as it's just an artifact of the browser file-upload procedure.

legrego

Thanks for making those first round changes @friol! I have some additional comments and thoughts for you

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx

x-pack/legacy/plugins/spaces/common/lib/dataurl.ts

x-pack/legacy/plugins/spaces/mappings.json

x-pack/legacy/plugins/spaces/common/lib/dataurl.ts

friol · 2019-09-11T17:52:15Z

@friol @ryankeairns do you think there's any value in persisting the image filename? IMO, storing the image alone should be sufficient.

I realize we display the filename when editing an existing space which contains a custom image, so we would need to figure out what to display there for that scenario, but it doesn't seem to add a whole lot of value to me. I also think it's awkward to ask API consumers to specify this, as it's just an artifact of the browser file-upload procedure.

@legrego : I had to persist the filename, as you said, because we have to show it if the space is saved and then reloaded. I tried to adhere to @ryankeairns 's mockup, also.
Filename and image are also optional, if I remember correctly, so you are not forced to save them.
But let me know what you prefer here.

ryankeairns · 2019-09-11T18:12:51Z

I don't think the filename adds much of value, no. The important piece is to allow for deleting/removing the image, so that you can get back to the colored/initials version.

How about this then (which might be circling back to where we started 😏):

friol · 2019-09-11T18:54:10Z

I don't think the filename adds much of value, no. The important piece is to allow for deleting/removing the image, so that you can get back to the colored/initials version.

How about this then (which might be circling back to where we started 😏):

@ryankeairns : that's my original idea, so... yes.

legrego · 2019-09-12T11:31:32Z

Sorry for the back and forth on the design @friol. Thanks for your patience while we converge our thinking 😄

legrego

We're very close! Just a couple small comments to address.
I also opened a PR against yours to add API docs. If you could incorporate these changes as well, that would be great!

I spent some time testing the API and UI, and everything appears to be working perfectly 👏

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx

x-pack/legacy/plugins/spaces/server/lib/space_schema.test.ts

legrego · 2019-09-17T11:50:14Z

@ryankeairns do you need/want to review & approve this PR from a design perspective?

elasticmachine · 2019-09-17T12:40:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ryankeairns

Design is looking good, just one small request to remove spacers. The interaction looks correct. Thank you!

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx

ryankeairns

LGTM. I will do some form design touchups in a separate PR as part of the compressed input effort.

Space Avatar - API doc updates

legrego · 2019-09-17T18:55:22Z

Jenkins test this

friol · 2019-09-17T18:58:41Z

@legrego: one question: commands like "jenkins test this" can only be issued by admins?

legrego · 2019-09-17T19:12:43Z

@friol yes, commands like that can only be issued by Elastic employees, to prevent abuse of our CI infrastructure.

legrego · 2019-09-17T19:20:47Z

Hey @elastic/kibana-docs! Is anyone available for a quick review of API doc changes?

I think the only interesting change is the description of the new field here:

kibana/docs/api/spaces-management/post.asciidoc

Lines 37 to 39 in d9c8e82

    
           `imageUrl`::  
        
             (Optional, string) Specifies the data-url encoded image to display in the space avatar. If specified, `initials` will not be displayed, and the `color` will be visible as the background color for transparent images. 
        
             For best results, your image should be 64x64. Images will not be optimized by this API call, so care should be taken when using custom images.

With identical text here:

kibana/docs/api/spaces-management/put.asciidoc

Lines 37 to 39 in d9c8e82

    
           `imageUrl`::  
        
             (Optional, string) Specifies the data-url encoded image to display in the space avatar. If specified, `initials` will not be displayed, and the `color` will be visible as the background color for transparent images. 
        
             For best results, your image should be 64x64. Images will not be optimized by this API call, so care should be taken when using custom images.

KOTungseth · 2019-09-17T19:29:55Z

docs/api/spaces-management/post.asciidoc

@@ -33,6 +33,10 @@ experimental[This functionality is *experimental* and may be changed or removed

 `color`:: 
  (Optional, string) Specifies the hexadecimal color code used in the space avatar. By default, the color is automatically generated from the space name.
+
+`imageUrl`:: 
+  (Optional, string) Specifies the data-url encoded image to display in the space avatar. If specified, `initials` will not be displayed, and the `color` will be visible as the background color for transparent images.


url should be URL

I've been changing every If specified to When specified, but totally up to you if it makes sense to change here.

To make the second sentence active, I would change it to "...initials are not displayed, and the color is visible as the background color for transparent images."

KOTungseth · 2019-09-17T19:31:36Z

docs/api/spaces-management/put.asciidoc

@@ -34,6 +34,10 @@ experimental[This functionality is *experimental* and may be changed or removed
 `color`:: 
  (Optional, string) Specifies the hexadecimal color code used in the space avatar. By default, the color is automatically generated from the space name.

+`imageUrl`:: 
+  (Optional, string) Specifies the data-url encoded image to display in the space avatar. If specified, `initials` will not be displayed, and the `color` will be visible as the background color for transparent images.


url should be URL

I've been changing every If specified to When specified, but totally up to you if it makes sense to change here.

To make the second sentence active, I would change it to "...initials are not displayed, and the color is visible as the background color for transparent images."

KOTungseth · 2019-09-17T19:33:40Z

docs/api/spaces-management/put.asciidoc

@@ -34,6 +34,10 @@ experimental[This functionality is *experimental* and may be changed or removed
 `color`:: 
  (Optional, string) Specifies the hexadecimal color code used in the space avatar. By default, the color is automatically generated from the space name.

+`imageUrl`:: 
+  (Optional, string) Specifies the data-url encoded image to display in the space avatar. If specified, `initials` will not be displayed, and the `color` will be visible as the background color for transparent images.
+  For best results, your image should be 64x64. Images will not be optimized by this API call, so care should be taken when using custom images.


To make the second sentence active, I would change it to "Images are not optimized by this API call."

Could we specify what "so care should be taken when using custom images" means? Or maybe change it to "Images are not optimized by this API call. Use custom images sparingly."

Could we specify what "so care should be taken when using custom images" means? Or maybe change it to "Images are not optimized by this API call. Use custom images sparingly."

How about something like this?
"Images are not optimized by this API call. Consider using the Spaces Management UI to have your images properly resized for efficient storage and display."

elasticmachine · 2019-09-17T20:41:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

legrego · 2019-09-18T10:46:34Z

@KOTungseth thanks for the speedy docs review! Since this is a community PR, and I contributed the docs, I will address your suggestions in a followup PR.

legrego

LGTM - thanks for all of your hard work on this, @friol!

legrego · 2019-09-18T12:11:14Z

@elasticmachine update branch

legrego · 2019-09-18T12:42:55Z

one last time, Jenkins test this

friol · 2019-09-18T12:45:53Z

LGTM - thanks for all of your hard work on this, @friol!

@legrego : thanks to both of you. Hope we'll do more in the future.

elasticmachine · 2019-09-18T14:18:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* First changes for avatar images * Added the ability to have custom images for space avatars * Partial changes as requested by reviewers * Final commit for space avatar images PR * Wrapping avatar file name * Colour picker always enabled, to allow background change for transparent svgs * All the changes requested in the last review * Fixes the type_check test errors * Fixing the rendering errors for space pages * Another batch of changes as requested by review * Some more snapshot tests * Last batch of changes * Fixed the type_check test * API doc updates * Removed comment * Removed imageUrl from state Co-authored-by: Larry Gregory <larry.gregory@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

friol added 2 commits September 9, 2019 07:02

First changes for avatar images

22874e8

Added the ability to have custom images for space avatars

af4d621

friol requested a review from a team as a code owner September 9, 2019 15:11

legrego self-requested a review September 9, 2019 15:21

legrego added Feature:Security/Spaces Platform Security - Spaces feature release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.0.0 labels Sep 9, 2019

legrego requested changes Sep 9, 2019

View reviewed changes

friol added 2 commits September 10, 2019 07:30

Partial changes as requested by reviewers

de8cea4

Final commit for space avatar images PR

e56ec19

Wrapping avatar file name

b53846f

Colour picker always enabled, to allow background change for transpar…

bb43112

…ent svgs

legrego self-requested a review September 11, 2019 11:59

legrego requested changes Sep 11, 2019

View reviewed changes

legrego reviewed Sep 17, 2019

View reviewed changes

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx Outdated Show resolved Hide resolved

x-pack/legacy/plugins/spaces/server/lib/space_schema.test.ts Outdated Show resolved Hide resolved

ryankeairns requested changes Sep 17, 2019

View reviewed changes

...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx Show resolved Hide resolved

ryankeairns self-requested a review September 17, 2019 14:52

ryankeairns approved these changes Sep 17, 2019

View reviewed changes

ryankeairns mentioned this pull request Sep 17, 2019

[META] Compressed panels release plan elastic/eui#2286

Closed

13 tasks

legrego mentioned this pull request Sep 17, 2019

Space Avatar - API doc updates friol/kibana#3

Merged

friol added 3 commits September 17, 2019 19:40

Merge pull request #3 from legrego/friol-avatar-docs

d9c8e82

Space Avatar - API doc updates

Removed comment

4d3f686

Removed imageUrl from state

1a8c8b1

legrego self-requested a review September 17, 2019 19:13

KOTungseth reviewed Sep 17, 2019

View reviewed changes

legrego approved these changes Sep 18, 2019

View reviewed changes

Merge branch 'master' into avatarimages

060eadb

legrego added the v7.5.0 label Sep 18, 2019

legrego merged commit 30a1730 into elastic:master Sep 18, 2019

legrego mentioned this pull request Sep 18, 2019

[7.x] Custom space avatar images (#45148) #46019

Merged

Custom space avatar images #45148

Custom space avatar images #45148

Conversation

friol commented Sep 9, 2019 • edited by legrego Loading

Summary

elasticmachine commented Sep 9, 2019

elasticmachine commented Sep 9, 2019

legrego commented Sep 9, 2019

elasticmachine commented Sep 9, 2019

legrego left a comment

Choose a reason for hiding this comment

ryankeairns commented Sep 10, 2019 • edited Loading

Text overflowing container; Disabled background color showing through an image with transparency

friol commented Sep 10, 2019

ryankeairns commented Sep 10, 2019

friol commented Sep 10, 2019

ryankeairns commented Sep 10, 2019

friol commented Sep 10, 2019

legrego commented Sep 11, 2019

legrego left a comment

Choose a reason for hiding this comment

friol commented Sep 11, 2019

ryankeairns commented Sep 11, 2019

friol commented Sep 11, 2019

legrego commented Sep 12, 2019

legrego left a comment • edited Loading

Choose a reason for hiding this comment

legrego commented Sep 17, 2019

elasticmachine commented Sep 17, 2019

💚 Build Succeeded

ryankeairns left a comment • edited Loading

Choose a reason for hiding this comment

ryankeairns left a comment

Choose a reason for hiding this comment

legrego commented Sep 17, 2019

friol commented Sep 17, 2019

legrego commented Sep 17, 2019

legrego commented Sep 17, 2019 • edited Loading

KOTungseth Sep 17, 2019

Choose a reason for hiding this comment

KOTungseth Sep 17, 2019

Choose a reason for hiding this comment

KOTungseth Sep 17, 2019

Choose a reason for hiding this comment

legrego Sep 17, 2019

Choose a reason for hiding this comment

elasticmachine commented Sep 17, 2019

💚 Build Succeeded

legrego commented Sep 18, 2019 • edited Loading

legrego left a comment

Choose a reason for hiding this comment

legrego commented Sep 18, 2019

legrego commented Sep 18, 2019

friol commented Sep 18, 2019

elasticmachine commented Sep 18, 2019

💚 Build Succeeded

friol commented Sep 9, 2019 •

edited by legrego

Loading

ryankeairns commented Sep 10, 2019 •

edited

Loading

legrego left a comment •

edited

Loading

ryankeairns left a comment •

edited

Loading

legrego commented Sep 17, 2019 •

edited

Loading

legrego commented Sep 18, 2019 •

edited

Loading