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

[Canvas] Move Templates to be stored as Saved Objects #69438

Merged
merged 7 commits into from Jun 30, 2020

Conversation

crob611
Copy link
Contributor

@crob611 crob611 commented Jun 17, 2020

Summary

This PR pulls the templates out of the client side and stores them as Saved Objects instead. This also sets us up to allow templates to be installed from elsewhere (package manager, direct import, etc). The saved objects are namespace agnostic, so they should show up for every space.

When Kibana starts, it will check to see if you have any Templates, and if not, it will create the 5 existing templates that we have. The templates are importable/exportable through Saved Object Management.

Dev Docs

Previously, Workpad Templates could be added through the Canvas API client side. Workpad Templates are now stored as Saved Objects, so there is no api needed for adding them. Any additional templates can simply be added through SavedObject management.

@crob611 crob611 added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. loe:x-large Extra Large Level of Effort v8.0.0 v7.9.0 labels Jun 17, 2020
@crob611 crob611 requested a review from a team as a code owner June 17, 2020 19:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

this looks good to me.

Do we need to think about a loading in the UI when a workpad is created from a template? Pitch pres takes about 1s on my MBP


import React, { useContext, useState, useEffect } from 'react';
import { RouterContext } from '../router';
// @ts-ignore Untyped Local
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use the new @ts-expect-error ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should.

attributes: template,
}));

client.bulkCreate(templateObjects);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a case where someone would want to permanently remove templates? I guess they can't delete templates now anyway..

},
},
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

do these large template files affect any of the bundling/build stuff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These previously existed, and here they are just moved to /server and also converted from plain json to ts so that they are properly typed, which will help if workpad the workpad type changes in the future. So these are not ending up in the bundle.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Nice work...!


export type PaginateChildProps = Omit<PaginateProps, 'children'>;

export const Paginate: React.FunctionComponent<InPaginateProps> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can import FC in the import statement above and shorten this line considerably.

});
export const Paginate: React.FunctionComponent<PaginateProps> = (props) => {
return (
<React.Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can use <> and </>

import PropTypes from 'prop-types';
import { PaginateProps } from './';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would put this props interface in this file and export the other direction... I've never liked how Redux expects the opposite. YMMV.

nextPage: props.nextPage,
prevPage: props.prevPage,
});
export const Paginate: React.FunctionComponent<PaginateProps> = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: destructure this object so you can avoid the props. in the object below.

return (
<React.Fragment>
{props.children({
rows: props.rows,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: doesn't this collection match props...? You might consider:


export const Paginate: FC<PaginateProps> = (props) => (
  <>{children(props)}</>
);

...or...

export const Paginate: FC<PaginateProps> = ({ 
  perPage, 
  pageNumber, 
  totalPages, 
  nexPageEnabled, 
  prevPageEnabled, 
  setPage, 
  nextPage, 
  prevPage, 
  ...rest,
}) => (
  <>
    {props.children({
      perPage, 
      pageNumber, 
      totalPages, 
      nexPageEnabled, 
      prevPageEnabled, 
      setPage, 
      nextPage, 
      prevPage,
    })}
  </>
);

perPage?: number;
startPage?: number;
rows: any[];
children: (props: PaginateChildProps) => React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider changing this to childFn... I was confused about this being a function producing children as opposed to a collection of children.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
canvas 790 -6 796

History

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

@crob611 crob611 merged commit a9f72bc into elastic:master Jun 30, 2020
Bamieh pushed a commit to Bamieh/kibana that referenced this pull request Jul 1, 2020
* Moves Canvas templates to live server side

* Adds Clone from template test

* Fix url

* Clean up

* PR Feedback

* i18n
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 69438 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 2, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 69438 or prevent reminders by adding the backport:skip label.

crob611 pushed a commit to crob611/kibana that referenced this pull request Jul 6, 2020
* Moves Canvas templates to live server side

* Adds Clone from template test

* Fix url

* Clean up

* PR Feedback

* i18n
# Conflicts:
#	x-pack/plugins/canvas/public/components/workpad_templates/workpad_templates.tsx
crob611 pushed a commit that referenced this pull request Jul 6, 2020
* Moves Canvas templates to live server side

* Adds Clone from template test

* Fix url

* Clean up

* PR Feedback

* i18n
# Conflicts:
#	x-pack/plugins/canvas/public/components/workpad_templates/workpad_templates.tsx
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loe:x-large Extra Large Level of Effort release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants