louisnw01 / lightweight-charts-python

Python framework for TradingView's Lightweight Charts JavaScript library.
MIT License
1.11k stars 208 forks source link

Add PanelChart widget #40

Closed MarcSkovMadsen closed 10 months ago

MarcSkovMadsen commented 1 year ago

HoloViz Panel is powerful data exploration & web app framework for Python. I'm a user and contributor. I work in energy trading and would like to be able to use the TradingView charts in my teams data apps.

Panel has a different architecture than Streamlit that makes it support streaming, async and bidirectional communication very well. It works both for exploration in notebooks and on web servers too.

Would you be interested in adding support for Panel? I would be willing to contribute. I already have a proof of concept, so I know it will work. I will primarily need your review and guidance. I would need to refactor the existing code a little bit.

https://github.com/louisnw01/lightweight-charts-python/assets/42288570/c906b7d6-0e67-443c-86c4-f149f6995a7d

https://github.com/louisnw01/lightweight-charts-python/assets/42288570/3e4cb0d4-9ddf-4cb3-a15b-75f413977b51

louisnw01 commented 1 year ago

Hey @marcskovmadsen,

This is really interesting! Glad there is a library that solves the main issue we’ve been having with Streamlit.

The changes you’ve made look good; still trying to wrap my head around all of them, but there is one that sticks out to me:

You are changing the id attribute of LWC in PanelChart to 'state.chart', which normally uses the convention of 'window.<random-str>', using the _rand.generate() method; I’m assuming this is necessary for Holoviz, however SubCharts, Lines (and HorizontalLines in the next update) will also need to be refactored as they also use the 'window.<random-str>' convention. Would self.id = f’state.{self._rand_generate()}’ work?

I noticed your also using the online distro of Lighweight Charts rather than the pkg.js included in the library, is there any reason for this?

Just preference, but instead of MakeChartCore you could add a wrapper parameter to MakeChart, which by default is document.getElementById(‘wrapper’).

Also, are there any concerns regarding security? One of the main reasons I was reluctant to add web support to this library was due to its reliance on evaluating JS within the DOM.

Anyway, I would love to implement this into the library, however I think it would be best to hold off for now as the next update (should be released next week) includes some relatively significant refactoring itself and adds support for drawings, and i’d prefer to merge this once that is all completed.

Thanks, and awesome work! Louis

MarcSkovMadsen commented 1 year ago

Thanks @louisnw01 👍

I think i can find solutions to all the your feedback. Really helpful. Thanks

MarcSkovMadsen commented 1 year ago

I like to extend my network when I find people with similar interests. Feel free to connect

s71m commented 3 months ago

oh it would be interesting to see this collaboration