Skip to content
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

Png #77930

Closed
wants to merge 3 commits into from
Closed

Png #77930

wants to merge 3 commits into from

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Sep 18, 2020

WIP

Notes:

  • navigator.clipboard available only on HTTPS or localhost. This can be detected, hide the action in that case.
  • Show user a notification on successful copy-to-clipboard.
  • Show error when something goes wrong.
  • Think about fallback or how to integrate this with "Download PNG". Notification message could contain:
    • Message saying that it was copied.
    • Preview of the chart.
    • "Download" button to download the PNG.

Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@streamich streamich requested a review from a team September 18, 2020 16:40
@streamich streamich requested a review from a team as a code owner September 18, 2020 16:40
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Sep 18, 2020
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 18, 2020

💔 Build Failed

Failed CI Steps

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
embeddable 93 +1 92

page load bundle size

id value diff baseline
embeddable 433.0KB +2.4KB 430.6KB
lens 1.1MB +3.1KB 1.1MB
uiActions 268.4KB +384.0B 268.1KB
total +5.9KB

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan tsullivan self-requested a review September 18, 2020 17:49
@flash1293
Copy link
Contributor

flash1293 commented Sep 21, 2020

@streamich The description still says WIP, but the PR isn't in draft mode - is this ready to review? Also, is there an issue for this feature for more context?

@@ -207,6 +209,9 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
embeddable: this,
});
}
if (isLensControlsEvent(event)) {
this.chartControls = event.data;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit weird seeing events pipeline used for this.
Maybe it is possible to forward a ref to controls instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, why do you think it is weird?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My take on this, not sure how much subjective is this, so curious about other's opinions:

I wouldn't expect that to get access to an api, I have to use an event system. Yes it works and can't be implemented as is, but getting it via reference to a child component seems like more common pattern to me?

I think using events for this is abusing them for things that there are not meant to handle:

  • Events: notifying about changes or interactions in a system and passing meta information about the change
  • Not events: exposing imperative handlers to underlying APIs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree this is weird ..... i would expect event.data to be serializable ....

why do we need to do this, why doesn't chart handle this internally ?

Copy link
Contributor Author

@streamich streamich Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... getting it via reference to a child component seems like more common pattern to me?

It is effectively a function reference, like:

<input ref={ref => {}} />

Functional references are better than named or object references.

i agree this is weird ..... i would expect event.data to be serializable ....

I'm not sure why you find it weird, it is a common pattern. If event.data had to be serializable, then we would need to use something else, but I'm not aware of this requirement.

why do we need to do this, why doesn't chart handle this internally ?

I'm not sure what you mean. We need to expose a way for embeddable to create a .png from a chart. The chart handles the part it needs to handle, the embeddable handles its part.

return toBlob(snapshot.blobOrDataUrl);
},
};
onControls(controls);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just call controls.toPngBlob() here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can, but that would generate a PNG of every chart on the dashboard at the moment they initially mount, not something you want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ehh ? onControls calls handlers.event() which in turn generates the png ... so instead of doing handlers.event generate the png.

Copy link
Contributor Author

@streamich streamich Sep 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my code .png is generated only when user clicks "Copy PNG" action in UI

onControls calls handlers.event() which in turn generates the png ...

hadlers.event() does not generate anything, it just stores the controls object in embeddable, and then embeddable can use those controls when it needs to, i.e. when user clicks "Copy PNG" in UI.

@@ -207,6 +209,9 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
embeddable: this,
});
}
if (isLensControlsEvent(event)) {
this.chartControls = event.data;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree this is weird ..... i would expect event.data to be serializable ....

why do we need to do this, why doesn't chart handle this internally ?

@streamich streamich closed this Sep 21, 2020
@@ -233,4 +238,10 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
}
}
}

async toPngBlob() {
Copy link
Contributor Author

@streamich streamich Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use something like:

getExportFunctions(): {toPngBlob?: () => Promise<Blob>; toHtml?: () => Promise<string>}

@@ -535,6 +536,10 @@ export interface LensBrushEvent {
name: 'brush';
data: TriggerContext<typeof SELECT_RANGE_TRIGGER>['data'];
}
export interface LensControlsEvent {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to expressions plugin.

Comment on lines +552 to +554
export function isLensControlsEvent(event: ExpressionRendererEvent): event is LensControlsEvent {
return event.name === 'controls';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this function or move it to expressions plugin.

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants