feat: add vega-lite #137

lyjeileen · 2024-05-16T07:14:25Z

Changes

Add vega-lite component, stories and tests

Note: I didn't handle updates in this PR.

Screenshots

kaseyvee · 2024-05-16T16:09:52Z

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

+  }, [])
+
+  if (hasError) {
+    return <Typography variant="body2">Failed to load the chart.</Typography>


Are there other ways of triggering the error? In storybook you use "invalid chart"- do we want to recommend to check the spec or schema?

Can't think of other error cases. I thought about mentioning invalid spec in the error message, but end users might not have any controls for that. What do you think?

End users might not be able to control directly but I'm thinking maybe it might help them try to refine their prompt? Like if speaking to say, an agent

I dont think we should worry too much about this. This error message is ok

kaseyvee · 2024-05-16T16:20:32Z

src/components/types.ts

 export type TableData = TableFormat & Updates<TableFormat>

+export interface VegaLiteChartFormat extends DataFormat {
+  /** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Need to use 'container' for width or height for responsive chart. */


Suggested change

/** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Need to use 'container' for width or height for responsive chart. */

/** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Set width or height to 'container' in the spec for a responsive chart. */

I think it would be nice if we could automatically do this

Are you suggesting having default value if width and height are not provided?

I was suggesting that we force the chart to be responsive by adding the required properties to the spec. Is that possible ?

kaseyvee · 2024-05-16T16:59:34Z

Maybe in a follow up but maybe we can look into applying the theme colours. I think changing background and text for now would be sufficient since dark mode looks like this:

Saw some examples here.

Shiti · 2024-05-16T16:37:28Z

src/components/types.ts

 export type TableData = TableFormat & Updates<TableFormat>

+export interface VegaLiteChartFormat extends DataFormat {
+  /** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Need to use 'container' for width or height for responsive chart. */


I think it would be nice if we could automatically do this

Shiti · 2024-05-16T16:47:29Z

src/components/types.ts


+export interface VegaLiteChartFormat extends DataFormat {
+  /** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Need to use 'container' for width or height for responsive chart. */
+  spec: VisualizationSpec


VisualizationSpec and DataFormat both have the fields title and description. How are the two handled/rendered differently ? Could we add an example ?

Shiti · 2024-05-16T16:50:48Z

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

+    if (chartRef.current && props.spec) {
+      embed(chartRef.current, props.spec)
+        .then(() => {
+          hasError && setHasError(false)


Suggested change

hasError && setHasError(false)

setHasError(false)

Shiti · 2024-05-16T16:51:41Z

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

+
+import Typography from '@mui/material/Typography'
+import React, { useEffect, useRef, useState } from 'react'
+import { default as embed } from 'vega-embed'


Suggested change

import { default as embed } from 'vega-embed'

import { default as VegaEmbed } from 'vega-embed'

Shiti · 2024-05-16T16:55:46Z

src/components/chart/vegaLiteChart/vegaLiteChart.stories.tsx

+  decorators,
+}
+
+export const ErrorBars = {


I dont think we need to add this. We dont need to cover all the charts vega-lite supports in the samples. Also, I got confused with the name of this story.

Shiti · 2024-05-16T16:58:06Z

src/components/chart/vegaLiteChart/vegaLiteChart.stories.tsx

+  decorators,
+}
+
+export const Scatterplot = {


We can remove this one as well. We dont need to have data files to indicate it works with vega-lite spec.

Shiti · 2024-05-16T17:02:08Z

src/components/types.ts


 export type TableData = TableFormat & Updates<TableFormat>

+export interface VegaLiteChartFormat extends DataFormat {


Would it be useful to provide option to configure theme as well ?
https://github.com/vega/vega-themes

Maybe we could use 'default' when light theme and 'dark' with dark theme by default.

the current theme can be checked using useTheme. for example,

const isDarkTheme = useTheme().palette.mode === 'dark';

Do we want to provide a theme props and let people override dark and light theme?

yes we could do that and set default values

Shiti · 2024-05-17T21:47:28Z

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

+
+VegaLiteChart.defaultProps = {
+  theme: {
+    dark: 'dark' as const,


Suggested change

dark: 'dark' as const,

dark: 'dark' as const,

light: 'default' as const,

Shiti · 2024-05-17T22:08:23Z

src/components/chart/vegaLiteChart/vegaLiteChart.stories.tsx

+        x: { field: 'a', type: 'nominal', axis: { labelAngle: 0 } },
+        y: { field: 'b', type: 'quantitative' },
+      },
+    },


I think we should update this vega-lite spec to have title and description as well and showcase how having both looks like. So, there should be two titles and descriptions. One which is a sub-heading and has a following paragraph from the DataFormat props and the second for the chart, focusing on the chart's title and description

Vega-lite's description is only for commenting purpose and won't be rendered, but I could add them here

Shiti · 2024-05-17T23:17:19Z

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

+    return <Typography variant="body2">Failed to load the chart.</Typography>
+  } else {
+    return (
+      <Stack direction="column" className="rustic-responsive-chart">


Suggested change

<Stack direction="column" className="rustic-responsive-chart">

<Stack direction="column" className="rustic-chart">

src/components/chart/vegaLiteChart/vegaLiteChart.tsx

src/components/chart/vegaLiteChart/vegaLiteChart.cy.tsx

src/components/chart/vegaLiteChart/vegaLiteChart.css

kaseyvee · 2024-05-18T00:01:59Z

src/components/chart/vegaLiteChart/vegaLiteChart.stories.tsx

+          'A theme object has the following fields:\n' +
+          '  light: A light theme string that is supported by vega-themes.\n' +
+          '  dark: A dark theme string that is supported by vega-themes.\n' +
+          'Refer to the [vega-themes](https://github.com/vega/vega-themes) documentation for more information.',


This part doesn't seem to be rendering correctly (the link). Maybe you can add it to the prop description instead.

I think vega also accepts an object. I dont think we should restrict it to string

kaseyvee

just moving the vega theme ref link to the prop description otherwise lgtm

kaseyvee reviewed May 16, 2024

View reviewed changes

lyjeileen requested a review from Shiti May 16, 2024 16:46

Shiti requested changes May 16, 2024

View reviewed changes

lyjeileen force-pushed the vega-lite branch 2 times, most recently from 44e5824 to 8a498d3 Compare May 17, 2024 21:25

Shiti reviewed May 17, 2024

View reviewed changes

lyjeileen requested review from florenceloi and milliondreams as code owners May 17, 2024 23:15

Shiti reviewed May 17, 2024

View reviewed changes

src/components/chart/vegaLiteChart/vegaLiteChart.tsx Outdated Show resolved Hide resolved

Shiti reviewed May 17, 2024

View reviewed changes

src/components/chart/vegaLiteChart/vegaLiteChart.cy.tsx Outdated Show resolved Hide resolved

Shiti reviewed May 17, 2024

View reviewed changes

src/components/chart/vegaLiteChart/vegaLiteChart.css Outdated Show resolved Hide resolved

Shiti previously approved these changes May 17, 2024

View reviewed changes

kaseyvee reviewed May 18, 2024

View reviewed changes

kaseyvee approved these changes May 18, 2024

View reviewed changes

lyjeileen added 5 commits May 17, 2024 17:08

feat: add vega-lite

1dbd684

fix: update based on suggestions

9744a51

fix: update based on comments

bb05f31

refactor: change css className

70ac0b1

docs: move link to description

2258b4d

lyjeileen dismissed Shiti’s stale review via 2258b4d May 18, 2024 00:14

lyjeileen force-pushed the vega-lite branch from a47c661 to 2258b4d Compare May 18, 2024 00:14

lyjeileen requested a review from Shiti May 18, 2024 00:15

Shiti approved these changes May 19, 2024

View reviewed changes

Shiti merged commit c3fb4fc into rustic-ai:main May 19, 2024

	/** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Need to use 'container' for width or height for responsive chart. */
	/** Follow Vega-lite's [documentation](https://vega.github.io/vega-lite/) to provide a specification object. Schema should be included in the spec. Set width or height to 'container' in the spec for a responsive chart. */

	import { default as embed } from 'vega-embed'
	import { default as VegaEmbed } from 'vega-embed'


		export type TableData = TableFormat & Updates<TableFormat>

		export interface VegaLiteChartFormat extends DataFormat {

	dark: 'dark' as const,
	dark: 'dark' as const,
	light: 'default' as const,

	<Stack direction="column" className="rustic-responsive-chart">
	<Stack direction="column" className="rustic-chart">

Uh oh!

feat: add vega-lite #137

feat: add vega-lite #137

Uh oh!

Conversation

lyjeileen commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Screenshots

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaseyvee May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaseyvee May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shiti May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaseyvee commented May 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaseyvee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

lyjeileen commented May 16, 2024 •

edited

Loading

kaseyvee May 16, 2024 •

edited

Loading

kaseyvee May 16, 2024 •

edited

Loading

Shiti May 16, 2024 •

edited

Loading