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

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/plugins/embeddable/public/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
panelNotificationTrigger,
PANEL_NOTIFICATION_TRIGGER,
} from './lib';
import { CopyPngAction } from './lib/actions/copy_png_action';

declare module '../../ui_actions/public' {
export interface TriggerContextMapping {
Expand Down Expand Up @@ -61,7 +62,6 @@ export const bootstrap = (uiActions: UiActionsSetup) => {
uiActions.registerTrigger(panelBadgeTrigger);
uiActions.registerTrigger(panelNotificationTrigger);

const actionApplyFilter = createFilterAction();

uiActions.registerAction(actionApplyFilter);
uiActions.registerAction(createFilterAction());
uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, new CopyPngAction());
};
54 changes: 54 additions & 0 deletions src/plugins/embeddable/public/lib/actions/copy_png_action.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import {
UiActionsActionDefinition,
UiActionsActionDefinitionContext as ActionDefinitionContext,
} from 'src/plugins/ui_actions/public';
import { IEmbeddable } from '..';

interface Context {
embeddable: IEmbeddable;
}

export const COPY_PNG_ACTION = 'COPY_PNG_ACTION';

declare const ClipboardItem: any;

export class CopyPngAction implements UiActionsActionDefinition<Context> {
public readonly id = COPY_PNG_ACTION;
public readonly order = 25;

public readonly getDisplayName = () => 'Copy PNG';
public readonly getIconType = () => 'copy';

public readonly isCompatible = async (
context: ActionDefinitionContext<Context>
): Promise<boolean> => {
return !!context.embeddable.toPngBlob;
};

public readonly execute = async (context: ActionDefinitionContext<Context>): Promise<void> => {
const { embeddable } = context;
if (!embeddable.toPngBlob) return;
const blob = await embeddable.toPngBlob();
const item = new ClipboardItem({ [blob.type]: blob });
await (navigator.clipboard as any).write([item]);
};
}
6 changes: 6 additions & 0 deletions src/plugins/embeddable/public/lib/embeddables/i_embeddable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,10 @@ export interface IEmbeddable<
* List of triggers that this embeddable will execute.
*/
supportedTriggers(): Array<keyof TriggerContextMapping>;

/**
* Generate .png image out of the embeddable. This method is optional, however,
* if it is implemented, the embeddable must return a .png image as a `Blob`.
*/
toPngBlob?(): Promise<Blob>;
}
1 change: 1 addition & 0 deletions src/plugins/ui_actions/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export {
ActionDefinition as UiActionsActionDefinition,
createAction,
IncompatibleActionError,
ActionDefinitionContext as UiActionsActionDefinitionContext,
} from './actions';
export { buildContextMenuForActions } from './context_menu';
export { Presentable as UiActionsPresentable } from './util';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import {
import { DOC_TYPE, Document, injectFilterReferences } from '../../persistence';
import { ExpressionWrapper } from './expression_wrapper';
import { UiActionsStart } from '../../../../../../src/plugins/ui_actions/public';
import { isLensBrushEvent, isLensFilterEvent } from '../../types';
import { isLensBrushEvent, isLensFilterEvent, isLensControlsEvent } from '../../types';
import { LensChartControls } from '../../xy_visualization/xy_expression';

export interface LensEmbeddableConfiguration {
expression: string | null;
Expand Down Expand Up @@ -63,6 +64,7 @@ export class Embeddable extends AbstractEmbeddable<LensEmbeddableInput, LensEmbe
private domNode: HTMLElement | Element | undefined;
private subscription: Subscription;
private autoRefreshFetchSubscription: Subscription;
private chartControls: LensChartControls;

private externalSearchContext: {
timeRange?: TimeRange;
Expand Down Expand Up @@ -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.

}
};

destroy() {
Expand All @@ -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>}

if (!this.chartControls) throw new Error('Chart controls not available.');
if (!this.chartControls.toPngBlob) throw new Error('Lens <Chart> .toPngBlob() not available.');
return await this.chartControls.toPngBlob();
}
}
11 changes: 10 additions & 1 deletion x-pack/plugins/lens/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
TriggerContext,
VALUE_CLICK_TRIGGER,
} from '../../../../src/plugins/ui_actions/public';
import { LensChartControls } from './xy_visualization/xy_expression';

export type ErrorCallback = (e: { message: string }) => void;

Expand Down Expand Up @@ -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.

name: 'controls';
data: LensChartControls;
}

export function isLensFilterEvent(event: ExpressionRendererEvent): event is LensFilterEvent {
return event.name === 'filter';
Expand All @@ -544,11 +549,15 @@ export function isLensBrushEvent(event: ExpressionRendererEvent): event is LensB
return event.name === 'brush';
}

export function isLensControlsEvent(event: ExpressionRendererEvent): event is LensControlsEvent {
return event.name === 'controls';
}
Comment on lines +552 to +554
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.


/**
* Expression renderer handlers specifically for lens renderers. This is a narrowed down
* version of the general render handlers, specifying supported event types. If this type is
* used, dispatched events will be handled correctly.
*/
export interface ILensInterpreterRenderHandlers extends IInterpreterRenderHandlers {
event: (event: LensFilterEvent | LensBrushEvent) => void;
event: (event: LensFilterEvent | LensBrushEvent | LensControlsEvent) => void;
}
35 changes: 33 additions & 2 deletions x-pack/plugins/lens/public/xy_visualization/xy_expression.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React, { useState, useEffect } from 'react';
import React, { useState, useEffect, useRef } from 'react';
import ReactDOM from 'react-dom';
import moment from 'moment';
import {
Expand Down Expand Up @@ -67,6 +67,7 @@ type XYChartRenderProps = XYChartProps & {
histogramBarTarget: number;
onClickValue: (data: LensFilterEvent['data']) => void;
onSelectRange: (data: LensBrushEvent['data']) => void;
onControls?: (controls: LensChartControls) => void;
};

export const xyChart: ExpressionFunctionDefinition<
Expand Down Expand Up @@ -186,6 +187,9 @@ export const getXyChartRenderer = (dependencies: {
histogramBarTarget={dependencies.histogramBarTarget}
onClickValue={onClickValue}
onSelectRange={onSelectRange}
onControls={(data) => {
handlers.event({ name: 'controls', data });
}}
/>
</I18nProvider>,
domNode,
Expand Down Expand Up @@ -218,6 +222,16 @@ export function XYChartReportable(props: XYChartRenderProps) {
);
}

export interface LensChartControls {
toPngBlob?: () => Promise<Blob>;
}

const toBlob = async (blobOrUrl: Blob | string): Promise<Blob> => {
if (typeof blobOrUrl === 'object') return blobOrUrl;
const response = await fetch(blobOrUrl);
return await response.blob();
};

export function XYChart({
data,
args,
Expand All @@ -227,10 +241,27 @@ export function XYChart({
histogramBarTarget,
onClickValue,
onSelectRange,
onControls,
}: XYChartRenderProps) {
const { legend, layers, fittingFunction, gridlinesVisibilitySettings } = args;
const chartTheme = chartsThemeService.useChartsTheme();
const chartBaseTheme = chartsThemeService.useChartsBaseTheme();
const chartRef = React.useRef<null | Chart>(null);

React.useEffect(() => {
if (!onControls) return;
const controls: LensChartControls = {
toPngBlob: async () => {
const snapshot = chartRef.current!.getPNGSnapshot({
backgroundColor: 'white',
pixelRatio: 2,
});
if (!snapshot) throw new Error('Could not generate PNG.');
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.

}, [onControls]);

const filteredLayers = layers.filter(({ layerId, xAccessor, accessors }) => {
return !(
Expand Down Expand Up @@ -352,7 +383,7 @@ export function XYChart({
};

return (
<Chart>
<Chart ref={chartRef}>
<Settings
showLegend={
legend.isVisible && !legend.showSingleSeries
Expand Down