Charts support #311

sinisaos · 2023-05-26T13:03:09Z

Related to #146. @dantownsend I created this PR so you can try it, change it and play with this interesting new feature.

codecov-commenter · 2023-05-28T08:39:31Z

Codecov Report

Merging #311 (da9fe50) into master (8731cad) will increase coverage by 0.38%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #311      +/-   ##
==========================================
+ Coverage   93.31%   93.70%   +0.38%     
==========================================
  Files           5        5              
  Lines         359      381      +22     
==========================================
+ Hits          335      357      +22     
  Misses         24       24

Impacted Files	Coverage Δ
piccolo_admin/translations/data.py	`100.00% <ø> (ø)`
piccolo_admin/endpoints.py	`93.08% <100.00%> (+0.46%)`	⬆️

dantownsend

Very interesting! I left some comments. I think it's mostly done - the main thing at the moment is getting the chart data to be returned dynamically.

dantownsend · 2023-06-27T23:24:45Z

admin_ui/package.json

+        "chart.js": "^4.3.0",
        "core-js": "^3.6.5",
        "flatpickr": "^4.6.9",
+        "highcharts": "^11.0.1",
        "is-svg": "^4.3.1",
        "js-cookie": "^2.2.1",
        "json-bigint": "^1.0.0",
        "ssri": "^8.0.1",
        "vue": "^2.7.14",
+        "vue-chartkick": "^0.6.1",


What's the interaction between chart.js, highcharts and vue-chartkick?

Is chart.js redundant? It looks like we're just using highcharts?

Have you tried chart.js? It's slightly smaller than highcharts, and it supports tree shaking, so we might get a smaller bundle size:

https://bundlephobia.com/package/[email protected]
https://bundlephobia.com/package/[email protected]

Also, the licensing on highcharts looks complicated, so I'd rather stay away from it. chart.js is MIT like Piccolo Admin.

https://github.com/chartjs/Chart.js/blob/master/LICENSE.md

My mistake. I tried both libraries, so it accidentally stayed in package.json. This PR uses highcharts, but I will switch to Chart.js in the next commit.

dantownsend · 2023-06-27T23:26:17Z

admin_ui/src/components/TableNav.vue

@@ -1,5 +1,5 @@
 <template>
-    <div>
+    <div class="sidebar_wrapper">


What's this for?

This is for a menu sidebar of equal width between the Home page and other pages. Now this is not the case because the sidebar menu on the Home page is wider than on other pages. I can remove it if you want?

dantownsend · 2023-06-27T23:27:49Z

admin_ui/src/router.ts

@@ -8,6 +8,7 @@ import Login from './views/Login.vue'
 import ChangePassword from './views/ChangePassword.vue'
 import RowListing from './views/RowListing.vue'
 import AddForm from './views/AddForm.vue'
+import GetCharts from './views/GetCharts.vue'


Maybe we just call this view Chart.vue?

I will change that.

dantownsend · 2023-06-27T23:30:13Z

docs/source/charts/index.rst

+Charts
+======
+
+Piccolo Admin can display different types of charts based on yours


Typo here: yours -> your.

dantownsend · 2023-06-27T23:30:28Z

docs/source/charts/index.rst

+
+.. literalinclude:: ./examples/app.py
+
+Piccolo Admin will then show a charts in the UI.


charts -> chart

dantownsend · 2023-06-27T23:34:17Z

piccolo_admin/endpoints.py

+
+    """
+
+    def __init__(self, title: str, chart_type: str, data: t.List[t.Any]):


This is defined as a dataclass - so we can drop the @dataclass decorator and just keep the __init__ method. With our docs, Sphinx handles normal Python classes better than dataclasses.

I think the type annotation for data is wrong. In the docstring it shows that we can pass in an async function.

No, we can pass the result of async or sync function which is a list of lists. I will change the annotation to t.List[t.List[t.Any]]. I will also update the docstring.

But if you just pass the result, the chart is no longer dynamic. I think each time the chart is viewed the data should be calculated from the function.

dantownsend · 2023-06-27T23:38:51Z

piccolo_admin/endpoints.py

+    title: str
+    chart_slug: str
+    chart_type: str
+    data: t.List[t.Any]


If the data returned a list of tuples ... we could define it more precisely like:

data: t.List[t.Tuple[str, t.Union[int, float, decimal.Decimal]]]

Presumably each element is a string, followed by some kind of number.

Returned data must be list of lists. I will change the annotation to t.List[t.List[t.Any]].

dantownsend · 2023-06-27T23:40:12Z

piccolo_admin/endpoints.py

+
+    def get_charts(self) -> t.List[ChartConfigResponseModel]:
+        """
+        Returns  all charts registered with the admin.


I think we have an extra space here -> Returns all

dantownsend · 2023-06-27T23:40:25Z

piccolo_admin/endpoints.py

+
+    def get_single_chart(self, chart_slug: str) -> ChartConfigResponseModel:
+        """
+        Returns single chart.


-> Returns a single chart

dantownsend · 2023-06-27T23:48:13Z

piccolo_admin/example.py

+    charts=[
+        ChartConfig(
+            title="Movie count",
+            chart_type="Pie",
+            data=[
+                ["George Lucas", 4],
+                ["Peter Jackson", 6],
+                ["Ron Howard", 1],
+            ],
+        ),
+        ChartConfig(
+            title="Director gender",
+            chart_type="Column",
+            data=[["Male", 7], ["Female", 3]],
+        ),
+    ],


I'd add examples for each chart we support - I think we support Pie, Line, Column, Bar and Area.

Also, I think the main missing piece is the data is hardcoded at the moment, but in the docs we show how the user can pass in an async function, so the data can be fetched dynamically.

Yes, you're right, the data is hardcoded because we can't run database queries in the piccolo admin example (piccolo_conf error), but we can use a coroutine or sync function in a normal application. I will change the test to run the hardcoded data from the coroutine in example.py so that the user can see an example of how to use it. Something like this

async def fetch_chart_data() -> t.List[t.List[t.Any]]: return [ ["George Lucas", 4], ["Peter Jackson", 6], ["Ron Howard", 1], ] chart_data = asyncio.run(fetch_chart_data()) APP = create_admin(charts=[ ChartConfig( title="Movie count Pie", chart_type="Pie", data=chart_data, ), ChartConfig( title="Movie count Line", chart_type="Line", data=chart_data, ), ChartConfig( title="Movie count Column", chart_type="Column", data=chart_data, ), ChartConfig( title="Movie count Bar", chart_type="Bar", data=chart_data, ), ChartConfig( title="Movie count Area", chart_type="Area", data=chart_data, ), ], )

I think the problem is the query is being run before the engine has been assigned to the tables.

If we make it dynamic then we won't have this problem.

async def fetch_chart_data() -> t.List[t.List[t.Any]]: return await Movie.select() ... APP = create_admin(charts=[ ChartConfig( title="Movie count Pie", chart_type="Pie", source=fetch_chart_data, ), ]

Then every time the chart is fetched from the API, we run the function to get the latest data.

I can make the change if you like - it's up to you.

Of course. That would be great. In the last commit I made your suggested changes. I think I covered everything except one thing I had trouble wrapping the chart pages in the BaseView (to have a sidebar) due to a weird Vue router warning
vue-router.esm.js:16 [vue-router] missing param for named route "chart": Expected "chartSlug" to be defined even though the chartSlug is defined in the router like this

{ path: '/charts/:chartSlug', name: 'chart', component: Chart, props: true },

so I left the DetailBaseView. It would be nice if you could look at that too. Thanks

OK, I'll create a branch, and will play around.

dantownsend · 2023-06-27T23:55:02Z

A couple of thoughts on the this screen:

Ideally the chart would take up more space if available.

Also, one some pages in Piccolo Admin we don't show the sidebar. I think for the charts it would be better to see the sidebar, as it makes it easier to jump between charts.

sinisaos · 2023-07-03T07:31:55Z

Used in #316

sinisaos added 4 commits May 13, 2023 14:59

add charts support

c3c19a2

Resolved merge conflict

9ec5ccc

fix Playwright tests

cafb5c4

fix charts in dark mode

9ff7282

Merge branch 'piccolo-orm:master' into charts_support

bc19e7c

dantownsend reviewed Jun 27, 2023

View reviewed changes

sinisaos added 2 commits June 29, 2023 10:13

Merge branch 'piccolo-orm:master' into charts_support

6d8909a

add suggested changes

da9fe50

dantownsend mentioned this pull request Jun 30, 2023

Chart experiments #316

Draft

sinisaos closed this Jul 3, 2023

sinisaos deleted the charts_support branch July 3, 2023 07:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charts support #311

Charts support #311

sinisaos commented May 26, 2023

codecov-commenter commented May 28, 2023 •

edited

Loading

dantownsend left a comment

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023

dantownsend Jun 28, 2023 •

edited

Loading

sinisaos Jun 28, 2023

dantownsend Jun 27, 2023

sinisaos Jun 28, 2023

dantownsend Jun 27, 2023

sinisaos Jun 28, 2023

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023

sinisaos Jun 29, 2023

dantownsend Jun 29, 2023

dantownsend Jun 27, 2023

sinisaos Jun 29, 2023

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023

dantownsend Jun 27, 2023 •

edited

Loading

sinisaos Jun 29, 2023

dantownsend Jun 29, 2023

dantownsend Jun 29, 2023

sinisaos Jun 29, 2023

dantownsend Jun 30, 2023

dantownsend commented Jun 27, 2023

sinisaos commented Jul 3, 2023


		.. literalinclude:: ./examples/app.py

		Piccolo Admin will then show a charts in the UI.


		"""

		def __init__(self, title: str, chart_type: str, data: t.List[t.Any]):

Charts support #311

Charts support #311

Conversation

sinisaos commented May 26, 2023

codecov-commenter commented May 28, 2023 • edited Loading

Codecov Report

dantownsend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend commented Jun 27, 2023

sinisaos commented Jul 3, 2023

codecov-commenter commented May 28, 2023 •

edited

Loading

dantownsend Jun 28, 2023 •

edited

Loading

dantownsend Jun 27, 2023 •

edited

Loading