-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Custom space avatar images #45148
Custom space avatar images #45148
Conversation
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? |
1 similar comment
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? |
Hey @friol, thanks for this PR! I'll review this over the next day or so, and respond with any feedback. |
Pinging @elastic/kibana-security |
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.
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!
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
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 - 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 |
@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. |
@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). |
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! |
@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. |
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.
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
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Outdated
Show resolved
Hide resolved
@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. |
@ryankeairns : that's my original idea, so... yes. |
Sorry for the back and forth on the design @friol. Thanks for your patience while we converge our thinking 😄 |
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.
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
Outdated
Show resolved
Hide resolved
@ryankeairns do you need/want to review & approve this PR from a design perspective? |
💚 Build Succeeded |
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.
...plugins/spaces/public/views/management/edit_space/customize_space/customize_space_avatar.tsx
Show resolved
Hide resolved
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.
LGTM. I will do some form design touchups in a separate PR as part of the compressed input effort.
Space Avatar - API doc updates
Jenkins test this |
@legrego: one question: commands like "jenkins test this" can only be issued by admins? |
@friol yes, commands like that can only be issued by Elastic employees, to prevent abuse of our CI infrastructure. |
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
With identical text here: kibana/docs/api/spaces-management/put.asciidoc Lines 37 to 39 in d9c8e82
|
@@ -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. |
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.
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."
@@ -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. |
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.
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."
@@ -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. |
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.
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."
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.
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."
💚 Build Succeeded |
@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. |
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.
LGTM - thanks for all of your hard work on this, @friol!
@elasticmachine update branch |
one last time, Jenkins test this |
💚 Build Succeeded |
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* 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 <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
As asked in Issue #43528, this PR introduces the possibility to add custom avatar images for spaces.
Resolves #43528