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

feat: Adds plugin-chart-handlebars #17903

Merged
merged 22 commits into from
Apr 26, 2022

Conversation

jdbranham
Copy link
Contributor

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 -
markup

Markdown -
markdown

@jdbranham jdbranham changed the title Jb/feat plugin handlebars Feature: Adds plugin-chart-handlebars Dec 31, 2021
@mayurnewase
Copy link
Contributor

This is so cool..
Approved workflows.

@villebro
Copy link
Member

villebro commented Jan 2, 2022

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.

@villebro villebro self-requested a review January 2, 2022 11:31
@codecov
Copy link

codecov bot commented Jan 4, 2022

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.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

@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!

}),
};

export const HandlebarsTemplateControlSetItem: ControlSetItem = {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

@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

type: 'DndMetricSelect',
};

export const PercentMetricsControlSetItem: ControlSetItem = {
Copy link
Member

Choose a reason for hiding this comment

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

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)

}),
};

export const HandlebarsTemplateControlSetItem: ControlSetItem = {
Copy link
Member

Choose a reason for hiding this comment

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

@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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

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
Copy link

raags commented Mar 1, 2022

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

@villebro
Copy link
Member

villebro commented Apr 5, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@villebro
Copy link
Member

Awesome @jdbranham ! ❤️

@villebro
Copy link
Member

@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
Copy link

raags commented Apr 12, 2022

Is it possible to include this in 1.5?

@jdbranham
Copy link
Contributor Author

Thanks @villebro - we're merged up!

@villebro
Copy link
Member

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

@villebro villebro changed the title Feature: Adds plugin-chart-handlebars feat: Adds plugin-chart-handlebars Apr 15, 2022
Adds missing propeties in test formData
@villebro
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

jdbranham commented Apr 22, 2022

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
Copy link
Member

Thanks @jdbranham! Fingers crossed! 🤞

@villebro
Copy link
Member

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

@jdbranham
Copy link
Contributor Author

also seeing this 🤔
image

@jdbranham
Copy link
Contributor Author

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

@jdbranham
Copy link
Contributor Author

jdbranham commented Apr 23, 2022

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
image

And when I add it back -
image

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Member

villebro commented Apr 25, 2022

@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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, finally! 😄👍🚀

@villebro villebro merged commit e632b82 into apache:master Apr 26, 2022
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
* 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>
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* 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>
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants