Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
12 changes: 12 additions & 0 deletions src/server/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
export interface KbnServer {
config: () => configObject;
}

export interface configObject {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

configObject => ConfigObject

get: (path: string) => any;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

hm, the aligning in this file didn't get auto corrected. Maybe because of the .d.ts extension.

can you update so it's only two space indentations? and the } on line 8 should have no spaces before it.

Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@
export const LayoutTypes = {
PRESERVE_LAYOUT: 'preserve_layout',
PRINT: 'print',
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { pdf } from './pdf';
import { groupBy } from 'lodash';
import { oncePerServer } from '../../../../server/lib/once_per_server';
import { screenshotsObservableFactory } from './screenshots';
import { getLayoutFactory } from './layouts';
import { createlayout } from './layouts/layout_factory';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

createlayout => createLayout


const getTimeRange = (urlScreenshots) => {
const grouped = groupBy(urlScreenshots.map(u => u.timeRange));
Expand All @@ -31,7 +31,6 @@ const formatDate = (date, timezone) => {
function generatePdfObservableFn(server) {
const screenshotsObservable = screenshotsObservableFactory(server);
const captureConcurrency = 1;
const getLayout = getLayoutFactory(server);

const urlScreenshotsObservable = (urls, headers, layout) => {
return Rx.from(urls).pipe(
Expand Down Expand Up @@ -68,7 +67,9 @@ function generatePdfObservableFn(server) {


return function generatePdfObservable(title, urls, browserTimezone, headers, layoutParams, logo) {
const layout = getLayout(layoutParams);

const layout = createlayout(server, layoutParams);

const screenshots$ = urlScreenshotsObservable(urls, headers, layout);

return screenshots$.pipe(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Size } from '../../../../../types';

export interface CaptureConfig {
zoom: number;
viewport: Size;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

-2 spaces for the }


export interface ViewZoomWidthHeight {
zoom: number;
width: number;
height: number;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same comment with this file and the space aligning. Looks like we'll have to update .d.ts files automatically. This will probably get hit by the linter when ci runs.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

export { createlayout } from './layout_factory';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

createlayout => createLayout

export { PrintLayout } from './print_layout';
export { PreserveLayout } from './preserve_layout';
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { Size } from '../../../../../types';
import { ViewZoomWidthHeight } from './index.d';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

shouldn't need the .d in the name, and I don't think you need to specify index too. Try:

import { ViewZoomWidthHeight } from '.';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For the record, this doesn't work because it resolves it to index.js which exists in the same location, so have to be explicit with index.d.ts. Maybe the file should just be named types.d.ts in that case. Which also matches our other types.d.ts file.


export interface PageSizeParams {
pageMarginTop: number;
pageMarginBottom: number;
pageMarginWidth: number;
tableBorderWidth: number;
headingHeight: number;
subheadingHeight: number;
}

export interface PdfImageSize {
width: number;
height?: number;
}

export abstract class Layout {
public id: string = '';

constructor(id: string) {
this.id = id;
}

public abstract getPdfImageSize(): PdfImageSize;

public abstract getPdfPageOrientation(): string | undefined;

public abstract getPdfPageSize(pageSizeParamsIn: PageSizeParams): string | Size;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would just call is pageSizeParams and drop the In part of the name. Not a common convention in our code base to suffix input parameters with In.


public abstract getViewport(itemsCount: number): ViewZoomWidthHeight;

public abstract getBrowserZoom(): number;

public abstract getBrowserViewport(): Size;

public abstract getCssOverridesPath(): string;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { KbnServer } from '../../../../../../../../src/server/index';
import { LayoutTypes } from '../../../common/constants';
import { Layout } from './layout';
import { PreserveLayout } from './preserve_layout';
import { PrintLayout } from './print_layout';

// you'll notice that we aren't passing the zoom at this time, while it'd be possible to use
// window.pixelDensity to figure out what the current user is seeing, if they're going to send the
// PDF to someone else, I can see there being benefit to using a higher pixel density, so we're
// going to leave this hard-coded for the time being

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment is only relevant for preserve_layout. Can be removed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Still need to remove this


interface LayoutParams {
id: string;
dimensions: {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

can do:

dimensions: Size

height: number;
width: number;
};
}

export function createlayout(server: KbnServer, LayoutParamsin: LayoutParams): Layout {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LayoutParamsin => layoutParams.

Should start with a lowercase since it's a variable, not a class name or type.

if (LayoutParamsin && LayoutParamsin.id === LayoutTypes.PRESERVE_LAYOUT) {
return new PreserveLayout(LayoutParamsin.id, LayoutParamsin.dimensions);
}

// this is the default because some jobs won't have anything specified
return new PrintLayout(server, LayoutParamsin.id);
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import path from 'path';
import { Size } from '../../../../../types';
import { Layout, PageSizeParams } from './layout';

const ZOOM: number = 2;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Zoom used to be a param that could be passed in from a caller, but 2 was the default. This makes it completely hardcoded.

Also, you've removed the explanatory comments: https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.js#L9-L12

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Intentional. This setting was never exposed to the user as configurable - #20091

I don't think we ever will expose it. The only reason we use it is to make sure the resolution is a little bigger on the screenshots.

From Brandon in that above issue:

The zoom is an undocumented setting that I intended to remove, but never got around to it.


export class PreserveLayout extends Layout {
public selectors = {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this can be readonly too

screenshot: '[data-shared-items-container]',
renderComplete: '[data-shared-item]',
itemsCountAttribute: 'data-shared-items-count',
timefilterFromAttribute: 'data-shared-timefilter-from',
timefilterToAttribute: 'data-shared-timefilter-to',
};

public readonly groupCount = 1;
private readonly height: number;
private readonly width: number;
private readonly scaledHeight: number;
private readonly scaledWidth: number;

constructor(id: string, size: Size) {
super(id);
this.height = size.height;
this.width = size.width;
this.scaledHeight = size.height * ZOOM;
this.scaledWidth = size.width * ZOOM;
}

public getCssOverridesPath() {
return path.join(__dirname, 'preserve_layout.css');
}

public getBrowserViewport() {
return {
height: this.scaledHeight,
width: this.scaledWidth,
};
}

public getBrowserZoom() {
return ZOOM;
}

public getViewport() {
return {
height: this.scaledHeight,
width: this.scaledWidth,
zoom: ZOOM,
};
}

public getPdfImageSize() {
return {
height: this.height,
width: this.width,
};
}

public getPdfPageOrientation() {
return undefined;
}

public getPdfPageSize(pagesizeparams: PageSizeParams) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

pagesizeparams => pageSizeParams

return {
height:
this.height +
pagesizeparams.pageMarginTop +
pagesizeparams.pageMarginBottom +
pagesizeparams.tableBorderWidth * 2 +
pagesizeparams.headingHeight +
pagesizeparams.subheadingHeight,
width: this.width + pagesizeparams.pageMarginWidth * 2 + pagesizeparams.tableBorderWidth * 2,
};
}
}
Loading