Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(plugin-chart-handlebars): initial commit #1390

Closed

Conversation

jdbranham
Copy link

@jdbranham jdbranham commented Oct 5, 2021

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

💔 Breaking Changes - None

🏆 Enhancements - New Handlebars plugin

📜 Documentation - slim 😅

image

@jdbranham jdbranham requested a review from a team as a code owner October 5, 2021 14:53
@vercel
Copy link

vercel bot commented Oct 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/DB2zm2mvPB4GGe1ftUCbFBxaU1n3
✅ Preview: https://superset-ui-git-fork-savantly-net-jb-handlebars-plugin-superset.vercel.app

@rusackas
Copy link
Member

rusackas commented Oct 5, 2021

This is awesome!!! Thanks for tackling this! A few initial thoughts/questions:

  1. Feel free to remove the default styling added by the plugin generator (the gray background, rounded corners, etc) - that was just to illustrate where one can stick in custom CSS, rather than a recommendation of actual design/styling.
  2. Speaking of styling, it might be possible (first enhancement request!) to add in a CSS editor (like we do on Dashboards) to style this thing in myriad ways. I.e. Handlebars could add bad/good classes, and the CSS could render things as red/green.
  3. Security issues - not sure if this provides any new vectors to grab data from the rest of a dashboard, or anything mischievous like that. Appears perfectly reasonable at first glance, but it's worth taking a close look at with that sort of mindset.

This is something that could prove VERY useful indeed. Love this contribution.

@michael-s-molina
Copy link
Contributor

@jdbranham I just want to congratulate you on the excellent source code documentation. 👏🏼

@jdbranham
Copy link
Author

@rusackas -

  1. Good point on the styling. I didn't really look at the defaults. But I'll tweak the defaults a bit.
  2. CSS editor - good idea! We can have a separate editor for the CSS, maybe with a simple default.
  3. JS currently gets stripped out using the SafeMarkdown viewer, but it would be cool to add some sandboxing or extension points. That would need a thoughtful approach 🤔

@michael-s-molina - thanks! although the documentation pretty much came from the yo generator and some code I borrowed from the table plugin :)

@rusackas
Copy link
Member

rusackas commented Oct 7, 2021

...it would be cool to add some sandboxing or extension points. That would need a thoughtful approach

100%. I was starting to look into a particular library that uses iframes for this sort of thing, and makes it easy to pass data (as props) through to them. There was a lot of CSS work to make it look as intended, though. I've been meaning to revisit this, and would be happy to set up a chat with you if you're interested in such things.

@jdbranham
Copy link
Author

100%. I was starting to look into a particular library that uses iframes for this sort of thing, and makes it easy to pass data (as props) through to them. There was a lot of CSS work to make it look as intended, though. I've been meaning to revisit this, and would be happy to set up a chat with you if you're interested in such things.

That sounds great! I'm on the superset slack - ping me anytime.
I've worked a lot on integration/plugin projects. Been tinkering on this one for a while now -
https://github.com/savantly-net/sprout-platform/

I really dig superset, and would love to refine this plugin more and maybe contribute in other areas.

@jdbranham
Copy link
Author

jdbranham commented Oct 8, 2021

Cleaned up default styling and separated CSS from Markdown/up

Markup -
image

Markdown -
image

@jdbranham
Copy link
Author

I don't have visibility to the Vercel details, but the status check is failing.
Is there something I need to do? Or what's the next step for PR approval/merge?

Thanks!

@rusackas
Copy link
Member

I was just wishing I had this plugin, and realized I totally dropped the ball on this thread! Sorry for the long delay!

These are the Vercel errors we're seeing. Looks like they might be pretty easy to resolve :)

image

@jdbranham
Copy link
Author

@rusackas - I've resolved the buildQuery issue, and the merge conflict, but I didn't see where validateNonEmpty was being used.
I don't see any issues with prettier locally and the chart plugin seems to be working ok.

I'm not sure why Vercel is still failing =\

@zhaoyongjie
Copy link
Contributor

zhaoyongjie commented Nov 11, 2021

@ jdbranham Could you rebase master then force push to the repository? I saw there are 100 files changed. Thanks for your patience.

@zhaoyongjie
Copy link
Contributor

@jdbranham there is a dependency resolving issue in there.
https://github.com/apache-superset/superset-ui/runs/4179937207?check_suite_focus=true

@jdbranham
Copy link
Author

Thanks @zhaoyongjie !
I think I can add "react-dom": "^16.13.1" to the peer dependencies to resolve it 🤔
I'll try that.

@villebro
Copy link
Contributor

It seems to be erroring out with this:
image

Let us know if you need help resolving it!

@jdbranham
Copy link
Author

My apologies! - I should've looked closer after the rebase.
For transparency, pagination controls are not implemented 😅

@villebro
Copy link
Contributor

@jdbranham so close now, but still some lint that might go away with npm run lint:fix:
image

@villebro
Copy link
Contributor

The codebase on this repo has been moved to the main Apache Superset repo, and consequently the repo is in the process of being archived. See the Superset Improvement Proposal for details: apache/superset#13013 . While all currently open issues and PRs will be closed, we encourage you to reopen this PR on the main repo, which should be as simple as moving over any code changes as follows:

  • core packages: packages/** (this repo) -> superset-frontend/packages/** (main repo)
  • plugins: plugins/** (this repo) -> superset-frontend/plugins/** (main repo)

If you need help with the migration, please post a message on the SIP or reach out on the community Slack.

@villebro villebro closed this Dec 10, 2021
@jdbranham
Copy link
Author

Thanks @villebro - I'll move it over to the main repo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants