You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/01/14 08:57:02 UTC
[GitHub] [superset] villebro commented on a change in pull request #17903: Feature: Adds plugin-chart-handlebars
villebro commented on a change in pull request #17903:
URL: https://github.com/apache/superset/pull/17903#discussion_r784660180
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/components/Handlebars/HandlebarsViewer.tsx
##########
@@ -0,0 +1,33 @@
+import { SafeMarkdown } from '@superset-ui/core';
+import Handlebars from 'handlebars';
+import moment from 'moment';
+import React, { useMemo, useState } from 'react';
+
+export interface HandlebarsViewerProps {
+ templateSource: string;
+ data: any;
+}
+
+export const HandlebarsViewer = ({
+ templateSource,
+ data,
+}: HandlebarsViewerProps) => {
+ const [renderedTemplate, setRenderedTemplate] = useState('');
+
+ useMemo(() => {
+ const template = Handlebars.compile(templateSource);
+ const result = template(data);
+ setRenderedTemplate(result);
+ }, [templateSource, data]);
Review comment:
I would add a try-catch block here to catch compilation errors. Something like
```typescript
const [error, setError] = useState('');
useMemo(() => {
try {
const template = Handlebars.compile(templateSource);
const result = template(data);
setRenderedTemplate(result);
setError('');
} catch (error) {
setRenderedTemplate('');
setError(error.message);
}
}, [templateSource, data]);
```
You could then add an Error component that displays the error, like
```typescript
const Error = styled.pre`
white-space: pre-wrap;
`;
if (error) {
return <Error>{error}</Error>;
}
```
This would produce something like this when the template has a syntax error:
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/components/Handlebars/HandlebarsViewer.tsx
##########
@@ -0,0 +1,33 @@
+import { SafeMarkdown } from '@superset-ui/core';
+import Handlebars from 'handlebars';
+import moment from 'moment';
Review comment:
nit: it appears `moment` isn't listed as a dependency. Since `moment` is a dependency in the monorepo, we can probably add it as a peer dependency in `package.json`
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx
##########
@@ -0,0 +1,60 @@
+import {
+ ControlConfig,
+ ControlSetItem,
+ CustomControlConfig,
+ sharedControls,
+} from '@superset-ui/chart-controls';
+import { t, validateNonEmpty } from '@superset-ui/core';
+import React from 'react';
+import { CodeEditor } from '../../components/CodeEditor/CodeEditor';
+import { ControlHeader } from '../../components/ControlHeader/controlHeader';
+
+interface HandlebarsCustomControlProps {
+ value: string;
+}
+
+const HandlebarsTemplateControl = (
+ props: CustomControlConfig<HandlebarsCustomControlProps>,
+) => {
+ const val = String(
+ props?.value ? props?.value : props?.default ? props?.default : '',
+ );
+
+ const updateConfig = (source: string) => {
+ props.onChange(source);
+ };
+ return (
+ <div>
+ <ControlHeader>{props.label}</ControlHeader>
+ <CodeEditor
+ theme="dark"
+ value={val}
+ onChange={source => {
+ updateConfig(source || '');
+ }}
+ />
+ </div>
+ );
+};
+const handlebarsTemplateControlConfig: ControlConfig<any> = {
+ ...sharedControls.entity,
+ type: HandlebarsTemplateControl,
+ label: t('Handlebars Template'),
+ description: t('A handlebars template that is applied to the data'),
+ default: `<ul class="data_list">
+ {{#each data}}
+ <li>{{this}}</li>
+ {{/each}}
+ </ul>`,
+ isInt: false,
+
+ validators: [validateNonEmpty],
+ mapStateToProps: ({ controls }) => ({
+ value: controls?.handlebars_template?.value,
+ }),
+};
+
+export const HandlebarsTemplateControlSetItem: ControlSetItem = {
+ name: 'handlebarsTemplate',
+ config: handlebarsTemplateControlConfig,
Review comment:
nit: maybe we could just inline the `handlebarsTemplateControlConfig ` config from above here?
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/style.tsx
##########
@@ -0,0 +1,55 @@
+import {
+ ControlConfig,
+ ControlSetItem,
+ CustomControlConfig,
+ sharedControls,
+} from '@superset-ui/chart-controls';
+import { t } from '@superset-ui/core';
+import React from 'react';
+import { CodeEditor } from '../../components/CodeEditor/CodeEditor';
+import { ControlHeader } from '../../components/ControlHeader/controlHeader';
+
+interface StyleCustomControlProps {
+ value: string;
+}
+
+const StyleControl = (props: CustomControlConfig<StyleCustomControlProps>) => {
+ const val = String(
+ props?.value ? props?.value : props?.default ? props?.default : '',
+ );
+
+ const updateConfig = (source: string) => {
+ props.onChange(source);
+ };
+ return (
+ <div>
+ <ControlHeader>{props.label}</ControlHeader>
+ <CodeEditor
+ theme="dark"
+ mode="css"
+ value={val}
+ onChange={source => {
+ updateConfig(source || '');
+ }}
+ />
+ </div>
+ );
+};
+const styleControlConfig: ControlConfig<any> = {
+ ...sharedControls.entity,
+ type: StyleControl,
+ label: t('CSS Styles'),
+ description: t('CSS applied to the chart'),
+ default: '',
+ isInt: false,
+
+ validators: [],
+ mapStateToProps: ({ controls }) => ({
+ value: controls?.handlebars_template?.value,
+ }),
+};
+
+export const StyleControlSetItem: ControlSetItem = {
+ name: 'styleTemplate',
+ config: styleControlConfig,
Review comment:
nit: also inline config here
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx
##########
@@ -0,0 +1,60 @@
+import {
+ ControlConfig,
+ ControlSetItem,
+ CustomControlConfig,
+ sharedControls,
+} from '@superset-ui/chart-controls';
+import { t, validateNonEmpty } from '@superset-ui/core';
+import React from 'react';
+import { CodeEditor } from '../../components/CodeEditor/CodeEditor';
+import { ControlHeader } from '../../components/ControlHeader/controlHeader';
+
+interface HandlebarsCustomControlProps {
+ value: string;
+}
+
+const HandlebarsTemplateControl = (
+ props: CustomControlConfig<HandlebarsCustomControlProps>,
+) => {
+ const val = String(
+ props?.value ? props?.value : props?.default ? props?.default : '',
+ );
+
+ const updateConfig = (source: string) => {
+ props.onChange(source);
+ };
+ return (
+ <div>
+ <ControlHeader>{props.label}</ControlHeader>
+ <CodeEditor
+ theme="dark"
+ value={val}
+ onChange={source => {
+ updateConfig(source || '');
+ }}
+ />
+ </div>
+ );
+};
+const handlebarsTemplateControlConfig: ControlConfig<any> = {
+ ...sharedControls.entity,
+ type: HandlebarsTemplateControl,
+ label: t('Handlebars Template'),
+ description: t('A handlebars template that is applied to the data'),
+ default: `<ul class="data_list">
+ {{#each data}}
+ <li>{{this}}</li>
+ {{/each}}
+ </ul>`,
+ isInt: false,
+
+ validators: [validateNonEmpty],
+ mapStateToProps: ({ controls }) => ({
+ value: controls?.handlebars_template?.value,
+ }),
+};
+
+export const HandlebarsTemplateControlSetItem: ControlSetItem = {
Review comment:
nit, I would probably make this camelCase instead of PascalCase:
```suggestion
export const handlebarsTemplateControlSetItem: ControlSetItem = {
```
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx
##########
@@ -0,0 +1,60 @@
+import {
+ ControlConfig,
+ ControlSetItem,
+ CustomControlConfig,
+ sharedControls,
+} from '@superset-ui/chart-controls';
+import { t, validateNonEmpty } from '@superset-ui/core';
+import React from 'react';
+import { CodeEditor } from '../../components/CodeEditor/CodeEditor';
+import { ControlHeader } from '../../components/ControlHeader/controlHeader';
+
+interface HandlebarsCustomControlProps {
+ value: string;
+}
+
+const HandlebarsTemplateControl = (
+ props: CustomControlConfig<HandlebarsCustomControlProps>,
+) => {
+ const val = String(
+ props?.value ? props?.value : props?.default ? props?.default : '',
+ );
+
+ const updateConfig = (source: string) => {
+ props.onChange(source);
+ };
+ return (
+ <div>
+ <ControlHeader>{props.label}</ControlHeader>
+ <CodeEditor
+ theme="dark"
+ value={val}
+ onChange={source => {
+ updateConfig(source || '');
+ }}
+ />
+ </div>
+ );
+};
+const handlebarsTemplateControlConfig: ControlConfig<any> = {
+ ...sharedControls.entity,
+ type: HandlebarsTemplateControl,
+ label: t('Handlebars Template'),
+ description: t('A handlebars template that is applied to the data'),
+ default: `<ul class="data_list">
+ {{#each data}}
+ <li>{{this}}</li>
+ {{/each}}
+ </ul>`,
+ isInt: false,
+
+ validators: [validateNonEmpty],
+ mapStateToProps: ({ controls }) => ({
+ value: controls?.handlebars_template?.value,
Review comment:
let's add
```typescript
{
...,
renderTrigger: true,
}
```
here to avoid having to retrigger the data request every time the template is updated (this make the chart update in real time when updating the template 🎉 )
##########
File path: superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/style.tsx
##########
@@ -0,0 +1,55 @@
+import {
+ ControlConfig,
+ ControlSetItem,
+ CustomControlConfig,
+ sharedControls,
+} from '@superset-ui/chart-controls';
+import { t } from '@superset-ui/core';
+import React from 'react';
+import { CodeEditor } from '../../components/CodeEditor/CodeEditor';
+import { ControlHeader } from '../../components/ControlHeader/controlHeader';
+
+interface StyleCustomControlProps {
+ value: string;
+}
+
+const StyleControl = (props: CustomControlConfig<StyleCustomControlProps>) => {
+ const val = String(
+ props?.value ? props?.value : props?.default ? props?.default : '',
+ );
+
+ const updateConfig = (source: string) => {
+ props.onChange(source);
+ };
+ return (
+ <div>
+ <ControlHeader>{props.label}</ControlHeader>
+ <CodeEditor
+ theme="dark"
+ mode="css"
+ value={val}
+ onChange={source => {
+ updateConfig(source || '');
+ }}
+ />
+ </div>
+ );
+};
+const styleControlConfig: ControlConfig<any> = {
+ ...sharedControls.entity,
+ type: StyleControl,
+ label: t('CSS Styles'),
+ description: t('CSS applied to the chart'),
+ default: '',
+ isInt: false,
+
+ validators: [],
+ mapStateToProps: ({ controls }) => ({
+ value: controls?.handlebars_template?.value,
Review comment:
also add `renderTrigger: true` here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org