feat: Adds plugin-chart-handlebars #17903

jdbranham · 2021-12-31T03:35:00Z

Adds a new plugin, that renders the data payload
by applying a customizable handlebars template

Reopening PR from old repo -
apache-superset/superset-ui#1390

💔 Breaking Changes - None

🏆 Enhancements - New Handlebars plugin

📜 Documentation - slim 😅

Markup -

Markdown -

mayurnewase · 2021-12-31T05:35:12Z

This is so cool..
Approved workflows.

villebro · 2022-01-02T11:31:26Z

Thanks @jdbranham so much for bringing in this PR from superset-ui to the main repo! Tagging myself for a review + will be reviewing+testing this in the coming days.

codecov · 2022-01-04T04:15:24Z

Codecov Report

Merging #17903 (bc3ee85) into master (3a231f6) will decrease coverage by 0.44%.
The diff coverage is 30.00%.

@@            Coverage Diff             @@
##           master   #17903      +/-   ##
==========================================
- Coverage   66.47%   66.03%   -0.45%     
==========================================
  Files        1681     1713      +32     
  Lines       64467    67651    +3184     
  Branches     6607     7655    +1048     
==========================================
+ Hits        42856    44672    +1816     
- Misses      19917    21084    +1167     
- Partials     1694     1895     +201

Flag	Coverage Δ
hive	`52.66% <ø> (ø)`
javascript	`51.79% <30.00%> (+0.64%)`	⬆️
mysql	`?`
postgres	`?`
presto	`52.51% <ø> (ø)`
python	`82.08% <ø> (-0.31%)`	⬇️
sqlite	`81.72% <ø> (ø)`
unit	`47.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...plugins/plugin-chart-handlebars/src/Handlebars.tsx	`0.00% <0.00%> (ø)`
...ars/src/components/Handlebars/HandlebarsViewer.tsx	`0.00% <0.00%> (ø)`
...-frontend/src/visualizations/presets/MainPreset.js	`100.00% <ø> (ø)`
...andlebars/src/components/CodeEditor/CodeEditor.tsx	`14.28% <14.28%> (ø)`
...n-chart-handlebars/src/plugin/controls/columns.tsx	`16.66% <16.66%> (ø)`
...n-chart-handlebars/src/plugin/controls/groupBy.tsx	`16.66% <16.66%> (ø)`
...ndlebars/src/plugin/controls/handlebarTemplate.tsx	`22.22% <22.22%> (ø)`
...gin-chart-handlebars/src/plugin/controls/style.tsx	`22.22% <22.22%> (ø)`
...gin-chart-handlebars/src/plugin/controls/shared.ts	`30.76% <30.76%> (ø)`
...hart-handlebars/src/plugin/controls/pagination.tsx	`33.33% <33.33%> (ø)`
... and 111 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a231f6...bc3ee85. Read the comment docs.

villebro

@jdbranham I absolutely ❤️LOVE❤️ this plugin! First review pass along with a few comments - if you wish I can push some commits to the PR, e.g. resolve the conflicts and apply those proposed changes. Let me know how you want to proceed!

...rset-frontend/plugins/plugin-chart-handlebars/src/components/Handlebars/HandlebarsViewer.tsx

superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx

villebro · 2022-01-14T08:53:15Z

superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx

+  }),
+};
+
+export const HandlebarsTemplateControlSetItem: ControlSetItem = {


nit, I would probably make this camelCase instead of PascalCase:

Suggested change

export const HandlebarsTemplateControlSetItem: ControlSetItem = {

export const handlebarsTemplateControlSetItem: ControlSetItem = {

If we make it camelCase, it would be different then the other ControlSetItems 🤔

https://github.com/apache/superset/pull/17903/files#diff-c424dd95a62b793c9f111b432e365b8cb478f2320675a7d76c05afc917c109dfR133-R153

Yes; since they instantiate the ControlSetItem type, I consider naming them like regular JS variables (camelCase) more appropriate than PascalCase. So I would recommend changing the case on all of these, as it's the common convention throughout the other control definitions.

@jdbranham here is an example of instances of ControlSetItem in another plugin which are camelCase: https://github.com/apache/superset/blob/master/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx

superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/style.tsx

villebro · 2022-02-16T10:42:51Z

superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/metrics.tsx

+  type: 'DndMetricSelect',
+};
+
+export const PercentMetricsControlSetItem: ControlSetItem = {


The convention has been to make all these camelCase, as they've really just variables (making them PascalCase makes it appear as if they're types/interfaces/classes/React components)

villebro · 2022-02-16T10:49:16Z

superset-frontend/plugins/plugin-chart-handlebars/src/plugin/controls/handlebarTemplate.tsx

+  }),
+};
+
+export const HandlebarsTemplateControlSetItem: ControlSetItem = {


@jdbranham here is an example of instances of ControlSetItem in another plugin which are camelCase: https://github.com/apache/superset/blob/master/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx

villebro

This has gotten conflicted again 🙁 Tested and it works really nicely now - after this is rebased and you've had the opportunity to check my last comment about camelCasing the controls then I think this should be good to go!

raags · 2022-03-01T13:26:09Z

@jdbranham would love to try this plugin - could you fix the conflicts?

villebro · 2022-04-05T07:52:59Z

@jdbranham I've fixed the conflicts and outstanding review comments, but I'm unable to push to your PR (I assume the branch is protected). I'd really love for this plugin to be merged in time for the 2.0 cut, so please let me know if I can help get this merged.

jdbranham · 2022-04-11T19:34:44Z

That's great @villebro - I've been too swamped to get back to it 😓

I should be able to check a box on this PR to allow superset maintainers the ability to push to the fork branch, but I don't see it.
Looking for an alternative way...

jdbranham · 2022-04-11T19:39:33Z

@villebro I just added you to my organization instead. That should work!

villebro · 2022-04-12T11:41:58Z

Awesome @jdbranham ! ❤️

villebro · 2022-04-12T11:50:17Z

@jdbranham hmm I'm still unable to push to the PR. But I created this temporary branch if you want to merge it into your branch: https://github.com/apache/superset/tree/jb/feat-plugin-handlebars . After that it should be good to be merged.

raags · 2022-04-12T12:02:14Z

Is it possible to include this in 1.5?

Jb/feat plugin handlebars

jdbranham · 2022-04-12T15:55:56Z

Thanks @villebro - we're merged up!

villebro · 2022-04-13T15:04:46Z

@jdbranham @raags if we can get this merged today I'm def gonna include it in 1.5!

Adds missing propeties in test formData

villebro · 2022-04-20T06:11:14Z

@jdbranham there's a few minor lint issues, here's the diff which should fix the remaining errors:

diff --git a/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/buildQuery.test.ts b/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/buildQuery.test.ts
index d7c0457dfa..ec0eb1bae2 100644
--- a/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/buildQuery.test.ts
+++ b/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/buildQuery.test.ts
@@ -7,8 +7,8 @@ describe('Handlebars buildQuery', () => {
     granularitySqla: 'ds',
     groupby: ['foo'],
     viz_type: 'my_chart',
-    width: '500px',
-    height: '500px'
+    width: 500,
+    height: 500,
   };

   it('should build groupby with series in form data', () => {
diff --git a/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/transformProps.test.ts
index f24d2c29f4..b5dc643529 100644
--- a/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/transformProps.test.ts
+++ b/superset-frontend/plugins/plugin-chart-handlebars/test/plugin/transformProps.test.ts
@@ -9,8 +9,9 @@ describe('Handlebars tranformProps', () => {
     granularitySqla: 'ds',
     metric: 'sum__num',
     groupby: ['name'],
-    width: '500px',
-    height: '500px'
+    width: 500,
+    height: 500,
+    viz_type: 'handlebars',
   };
   const chartProps = new ChartProps<QueryFormData>({
     formData,

villebro · 2022-04-21T12:44:02Z

@jdbranham I'm probably going to cut 1.5.0rc4 in ~20 hours, so if you can push this last change in before then, I'll include this in 1.5.0 🚀

jdbranham · 2022-04-22T01:41:21Z

hope I made it in time!

My local config probably needs work - since these linting error should've been caught when I ran the tests 🤔

villebro · 2022-04-22T06:22:38Z

Thanks @jdbranham! Fingers crossed! 🤞

villebro · 2022-04-22T06:43:14Z

@jdbranham oh no, those widths and heights needed to be numbers (not strings), so the linter is still failing 😢

jdbranham · 2022-04-22T07:17:14Z

also seeing this 🤔

jdbranham · 2022-04-22T14:16:49Z

It's strange because when I run the tests locally the result is different than GHA 😢

jdbranham · 2022-04-23T02:15:50Z

I must have a problem with my environment. The tests in GHA show the exact opposite when I run locally.
Unless I'm comparing the wrong runs... I'll double check.

When I remove viz_type

And when I add it back -

Maybe I'm missing something silly, but I don't see viz_type in the type hierarchy, but I see it in a lot of other plugins' tests.

revert the viz_type change. it was in the wrong place.

MarcusSorealheis · 2022-04-23T04:06:19Z

My old homie @jdbranham I'm going to checkout this PR and see if I can reproduce to fix the issue. It's such a great feature.

Can you share a bit of info about your environment? I'm guessing you won't see this today or be at your computer but knowing you...

jdbranham · 2022-04-23T06:39:34Z

@MarcusSorealheis ! - I didn't know you were contributor. You're involved in all the cool stuff! =)

It's Friday night - what else is there to do 😅
Thanks for taking a look.

❯ node -v
v16.14.2

❯ npm -v
8.5.0

❯ arch
arm64

villebro · 2022-04-25T15:10:12Z

@jdbranham I tried to open a PR against your fork, but it got really messy (probably due to GH choking on too many forks). But this commit should fix the licence header CI issues + fix the unit test: bc3ee85

villebro

LGTM, finally! 😄👍🚀

* adds: plugin chart handlebars * adds: handlebars plugin to main presets * update: npm install * chore: lint * adds: dateFormat handlebars helper * deletes: unused props * chore: linting plugin-chart-handlebars * docs: chart-plugin-handlebars * adds: moment to peer deps * update: use error handling * update: inline config, adds renderTrigger * update: inline config, adds renderTrigger * camelCase controls * (plugins-chart-handlebars) adds: missing props Adds missing propeties in test formData * (plugin-chart-handlebars) fixes test * (plugin-handlebars-chart) use numbers for size * (feature-handlebars-chart) fix viz_type * (plugin-handlebars-chart) revert revert the viz_type change. it was in the wrong place. * fix test and add license headers Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>

jdbranham added 3 commits December 30, 2021 20:44

adds: plugin chart handlebars

26216de

adds: handlebars plugin to main presets

040f875

update: npm install

e0f35fc

pull-request-size bot added the size/XXL label Dec 31, 2021

jdbranham changed the title ~~Jb/feat plugin handlebars~~ Feature: Adds plugin-chart-handlebars Dec 31, 2021

jdbranham added 3 commits December 31, 2021 12:51

chore: lint

01aeee2

adds: dateFormat handlebars helper

0124a4c

deletes: unused props

76441d5

villebro self-requested a review January 2, 2022 11:31

jdbranham added 2 commits January 2, 2022 13:23

chore: linting plugin-chart-handlebars

cc53b4c

docs: chart-plugin-handlebars

5977ede

villebro reviewed Jan 14, 2022

View reviewed changes

jdbranham added 4 commits January 15, 2022 17:05

adds: moment to peer deps

4127184

update: use error handling

e1a7e9d

update: inline config, adds renderTrigger

575b6db

update: inline config, adds renderTrigger

6fc45cb

villebro reviewed Feb 16, 2022

View reviewed changes

villebro added 2 commits April 5, 2022 10:09

Merge branch 'master' into jb/feat-plugin-handlebars

51e1326

camelCase controls

3b227f8

Merge branch 'master' into jb/feat-plugin-handlebars

81342c6

Merge pull request #3 from apache/jb/feat-plugin-handlebars

285f2f3

Jb/feat plugin handlebars

villebro changed the title ~~Feature: Adds plugin-chart-handlebars~~ feat: Adds plugin-chart-handlebars Apr 15, 2022

(plugins-chart-handlebars) adds: missing props

292adb1

Adds missing propeties in test formData

jhult mentioned this pull request Apr 20, 2022

[chart] Replace Big Number with Big Card #12516

Closed

(plugin-chart-handlebars) fixes test

0a89abb

(plugin-handlebars-chart) use numbers for size

13c4ffe

(feature-handlebars-chart) fix viz_type

ea34336

(plugin-handlebars-chart) revert

7106b94

revert the viz_type change. it was in the wrong place.

fix test and add license headers

bc3ee85

villebro approved these changes Apr 26, 2022

View reviewed changes

villebro merged commit e632b82 into apache:master Apr 26, 2022

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adds plugin-chart-handlebars #17903

feat: Adds plugin-chart-handlebars #17903

jdbranham commented Dec 31, 2021

mayurnewase commented Dec 31, 2021

villebro commented Jan 2, 2022

codecov bot commented Jan 4, 2022 •

edited

villebro left a comment

villebro Jan 14, 2022

jdbranham Jan 15, 2022

villebro Jan 16, 2022

villebro Feb 16, 2022

villebro Feb 16, 2022

villebro Feb 16, 2022

villebro left a comment

raags commented Mar 1, 2022

villebro commented Apr 5, 2022

jdbranham commented Apr 11, 2022

jdbranham commented Apr 11, 2022

villebro commented Apr 12, 2022

villebro commented Apr 12, 2022

raags commented Apr 12, 2022

jdbranham commented Apr 12, 2022

villebro commented Apr 13, 2022

villebro commented Apr 20, 2022

villebro commented Apr 21, 2022

jdbranham commented Apr 22, 2022 •

edited

villebro commented Apr 22, 2022

villebro commented Apr 22, 2022

jdbranham commented Apr 22, 2022

jdbranham commented Apr 22, 2022

jdbranham commented Apr 23, 2022 •

edited

MarcusSorealheis commented Apr 23, 2022

jdbranham commented Apr 23, 2022

villebro commented Apr 25, 2022 •

edited

villebro left a comment

	export const HandlebarsTemplateControlSetItem: ControlSetItem = {
	export const handlebarsTemplateControlSetItem: ControlSetItem = {

feat: Adds plugin-chart-handlebars #17903

feat: Adds plugin-chart-handlebars #17903

Conversation

jdbranham commented Dec 31, 2021

mayurnewase commented Dec 31, 2021

villebro commented Jan 2, 2022

codecov bot commented Jan 4, 2022 • edited

Codecov Report

villebro left a comment

Choose a reason for hiding this comment

villebro Jan 14, 2022

Choose a reason for hiding this comment

jdbranham Jan 15, 2022

Choose a reason for hiding this comment

villebro Jan 16, 2022

Choose a reason for hiding this comment

villebro Feb 16, 2022

Choose a reason for hiding this comment

villebro Feb 16, 2022

Choose a reason for hiding this comment

villebro Feb 16, 2022

Choose a reason for hiding this comment

villebro left a comment

Choose a reason for hiding this comment

raags commented Mar 1, 2022

villebro commented Apr 5, 2022

jdbranham commented Apr 11, 2022

jdbranham commented Apr 11, 2022

villebro commented Apr 12, 2022

villebro commented Apr 12, 2022

raags commented Apr 12, 2022

jdbranham commented Apr 12, 2022

villebro commented Apr 13, 2022

villebro commented Apr 20, 2022

villebro commented Apr 21, 2022

jdbranham commented Apr 22, 2022 • edited

villebro commented Apr 22, 2022

villebro commented Apr 22, 2022

jdbranham commented Apr 22, 2022

jdbranham commented Apr 22, 2022

jdbranham commented Apr 23, 2022 • edited

MarcusSorealheis commented Apr 23, 2022

jdbranham commented Apr 23, 2022

villebro commented Apr 25, 2022 • edited

villebro left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2022 •

edited

jdbranham commented Apr 22, 2022 •

edited

jdbranham commented Apr 23, 2022 •

edited

villebro commented Apr 25, 2022 •

edited