Image Embeddable #146421
Image Embeddable #146421
Conversation
This reverts commit 26ca94e.
…ana into d/2022-10-24-image-embeddable
|
@gchaps, I updated the screenshots in the description with the latest copy |
|
@elasticmachine merge upstream |
…age-embeddable # Conflicts: # src/plugins/dashboard/public/application/top_nav/dashboard_top_nav.tsx # src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx # src/plugins/dashboard/public/dashboard_strings.ts
Dosant
force-pushed
the
d/2022-10-24-image-embeddable
branch
from
e37dc6c
to
91bd7ac
Compare
There was a problem hiding this comment.
Read through the code and also tested this locally. Code LGTM, but I left a few nits and a couple questions.
I also ran this locally and ran into some small issues. Nothing major though, so am approving.
- When you're editing an image, the save button at the bottom still says
Edit image. Usually we have some sort ofconfirm, orsavedown here. Not sure what @gchaps said about that, but it seemed strange to me. - If you open the editor, then cancel, there is an error in the console
- While typing into the URL section, each keystroke trIes to request the image which results in some flashing of the error state, potentially this could be at least debounced.
- By default the background color is transparent, but if you change the color you cannot set it back to transparent as far as I can tell.
- I anticipate that users will want to show an image with a transparent background, and avoid all the panel styling that Dashboard embeddables come with. We should maybe expose an option to not show the panel shading / background color etc.
| }, | ||
| }) | ||
| : i18n.translate('dashboard.addPanel.panelAddedToContainerSuccessMessageTitle', { | ||
| defaultMessage: 'A panel was added', |
There was a problem hiding this comment.
Do we need a toast when the panel is added? Adding other panels like this doesn't show a toast, this is only used when you add Library visualizations.
There was a problem hiding this comment.
I moved the code that adds an embeddable and calls this toast from src/plugins/dashboard/public/dashboard_app/top_nav/editor_menu.tsx to src/plugins/dashboard/public/dashboard_app/top_nav/dashboard_editing_toolbar.tsx to use it for creating image embeddable. The toast came with the code.
Do you think I should add an if/else there to not show a toast in case this wasn't a library viz? I personally not sure why the behavior has to be different.
There was a problem hiding this comment.
Ah, makes sense! I didn't realize that you standardized these calls. Thanks Anton!
src/plugins/dashboard/public/dashboard_app/top_nav/dashboard_editing_toolbar.tsx
Outdated
Show resolved
Hide resolved
src/plugins/embeddable/public/lib/embeddables/embeddable_factory.ts
Outdated
Show resolved
Hide resolved
| ); | ||
| const inDashboardEditMode = embeddable.getInput().viewMode === ViewMode.EDIT; | ||
| return Boolean(canEditEmbeddable && inDashboardEditMode); | ||
| } | ||
|
|
||
| public async execute(context: ActionContext) { | ||
| const embeddable = context.embeddable; | ||
| const { editableWithExplicitInput } = embeddable.getOutput(); |
There was a problem hiding this comment.
Is there a reason this is exposed via embeddable output rather than as either a method call or as a property on the embeddable definition? Do you expect this to change during the life time of individual embeddables?
It looks like this matches the current editable behaviour, I wonder if we have any instances where this changes.
There was a problem hiding this comment.
I am not sure! As you said, I followed the editable and other edit* related props pattern.
....
Looked around a bit:
Seems like editable can be dynamic, for example:
private getIsEditable() {
return (
this.deps.capabilities.canSaveVisualizations ||
(!this.inputIsRefType(this.getInput()) && this.deps.capabilities.canSaveDashboards)
);
}
editUrl is also can be dynamic depending on savedObjectId, for example.
editApp don't think so. and editableWithExplicitInput also not at least at the moment.
But I think makes sense to keep these properties together.
There was a problem hiding this comment.
Sounds good for the moment! Eventually we're hoping to remove the extra step of accessing these sorts of things via output, and standardize on calling them as methods on the embeddable instance, like getIsEditable. But that process doesn't have to start quite yet!
@ThomThomson, thank you for the review!
I followed @gchaps's recommendation to change from "save" to "add/edit". I didn't consider it is "Save" for all other flyouts. For consistency, considering to change it back.
Going to fix this.
I remembered that I decided to not worry about this after I realized that this is more of a dev/testing edge-case, as in the real world you will likely copy paste a URL from somewhere and won't type it.
You can by removing the color. Either in the input with the keyboard or by clicking "remove" button in the input
Probably. In case there will be interested, I think we will be able to add something like this. Maybe this could be useful for some other panel types |
There was a problem hiding this comment.
This is look really great @Dosant . I read over the code fairly quickly, from what I can see the implementation is looking very clean. Only minor comments. Did not really focus on integration with Dashboard or use of embeddable code.
I had a few UX comments I wanted to get your thoughts on:
At the moment we have tabbed content for "upload" and "use link" flows. What do you think about not having tabs and instead have some kind of field toggle for "use link" at the top of the form that will just toggle the upload field to a text field?
Minor: it looks like the "Or use a previously uploaded image" button link is not being disabled during upload.
Another minor: currently this places a lot of emphasis on the upload new image flow. If we want to present them as equal options we could place them next to each other, but that would require some redesign.
I also see what you mean regarding the "Cancel" button that pops in and causing a layout shift. I wonder if we could solve this by only showing the image configuration fields when the image is uploaded.
I would consider change this text to "Done". "Edit image" is a bit unclear to me as the label of the primary action button in this context. Although I'd be curious to hear what a writer thinks.
NOTE: this comment makes most sense if we also consider the field toggle approach I described above.
At the moment, we have the image "field" and the image "preview" functioning as the same thing. I wonder if we kept the image "field" functioning just as a field we could move the "preview" down to the bottom of the flyout. This introduces a new state for the upload field since it will need to, for example, just contain the image name. But it could look like:
Let me know what you think! None of these need to be actioned of course.
| validateUrl: ValidateUrlFn; | ||
| } | ||
|
|
||
| export function ImageEditorFlyout(props: ImageEditorFlyoutProps) { |
There was a problem hiding this comment.
Did you consider using the form lib here?
src/plugins/es_ui_shared/static/forms/docs/welcome.mdx
It comes with some pre-made fields and takes care of a lot of the awkward "in-between" states a form can be in.
Perhaps not worth doing now, just wanted to get your thoughts.
There was a problem hiding this comment.
Could probably also split out some of the JSX to smaller, sub-components.
There was a problem hiding this comment.
Did you consider using the form lib here?
I thought I'd start with plain React and if it starts to get weird, I'd try a form lib. But I don't think I hit that point with the current implementation where I felt that starting using a form worse it.
Could probably also split out some of the JSX to smaller, sub-components.
Same thinking here. I though I'd start creating sub-components when I'd feel specific fragments are becoming bloated and/or for re-render perf optimization reasons. but I don't think I hit that point.
Most of the components would mostly be Inputs with texts wrapped into components and I didn't think it worse it.
| * Makes sure the container has not too large height to preserve vertical space for the image configuration in the flyout | ||
| */ | ||
| const CONTAINER_SIZING_CSS = css({ | ||
| aspectRatio: `21 / 9`, |
There was a problem hiding this comment.
just curious, how did we land on this number?
There was a problem hiding this comment.
I started with 16 / 9 (common aspect-ratio everyone used at) but then had concerns that on some screens and sizes image placeholder takes to much height. So I picked another common aspect-ratio which would have less height.
There was a problem hiding this comment.
Code LGTM, tested on Mac/Chrome all seems good 🚀
Regarding drilldown integration, what is the latest there? Did we decide not to do them or that would be a separate work item?
Not directly related to this PR, but there is this "Customize time range" action and some other actions, which maybe should not appear on image panels.
There was a problem hiding this comment.
Thanks for including the updated screenshots. Looks good. Here are a few minor changes.
Remove the word “Or”
Use a previously uploaded image
Add serial comma to hint text
Supported file types: png, jpeg, web, and avif.
Remove URL from message
Unable to load image.
It’s the same as in the text field directly above the message and it just makes the message harder to read.
|
@jloleysens, thank you for the feedback and idea.
This was changed
This jump is annoying, but I don't think it worth changing the form flow.
I think these are really good ideas! I agree that combined this could be a really good alternative UX with bunch of advantages over the current version. |
I didn't get to it in this PR. I'll create a separate issue and will time box like a day to add it
Good catch. I'll try to take a look |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled in files
ESLint disabled line counts
miscellaneous assets size
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Dosant
added
impact:medium
close elastic#81345 Adds an image embeddable - a new embeddable type that allows to insert images into dashboard using the new file service
Just created it :) elastic/eui#6509 |
close #81345
Summary
This PR adds an image embeddable - a new embeddable type that allows to insert images into dashboard.
Feature Review
Adding an Image panel
To add an image embeddable to a dashboard use a shortcut "add image" button or an panel type picker
Image editor opens in a flyout without navigating away from a Dashboard app.
First pick an image source: either upload a file or add an external URL
When using "upload" option:
When imaged picked or uploaded it is displayed in a preview container:
Users can also add images by providing external URL
Url validation checks the URL using
http.externalUrlservice and expects an absolute URL overhttps?protocolsIn case URL is invalid or doesn't point to an image, error is displayed and preview container shows not found state
Image Panel on a Dashboard
If image URL validation passes and image loads the image displays as configured:
In case panel doesn't have panel title, image can be displayed nicely occupying the whole panel
In case of image load error or validation error, we display an error state. Visually it depends on screen size and panel size:
Original image is visually hidden, so screen readers can still read alt text in this case.
TODO
This PR
Later (create issues)
Checklist
Delete any items that are not applicable to this PR.