-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/charts package #1648
base: main
Are you sure you want to change the base?
Feature/charts package #1648
Conversation
Deployment failed with the following error:
View Documentation: https://vercel.com/docs/accounts/team-members-and-roles |
packages/charts/src/theme/colors.ts
Outdated
export const CATEGORICAL = { | ||
primary: '#014592', | ||
secondary: '#9C56F3', | ||
tertiary: '#0D504D', | ||
quaternary: '#CA226A', | ||
quinary: '#F95D47', | ||
senary: '#5C12B6', | ||
septenary: '#08A822', | ||
octonary: '#EF5997', | ||
nonary: '#157BF4', | ||
denary: '#B18D01', | ||
undenary: '#013A5E', | ||
duodenary: '#01A29B;', | ||
ternary: '#B24D01', | ||
fourteen: '#720000', | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me that this naming could be simpler. For example: 100, 200, 300, 400, 500... OR 1, 2, 3, 4, 5... You would write less and semantic actually gets lost after quaternary, for example. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can achieve a better way to name it, for now i'm was following the token names defined in Figma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about this naming, @beatrizmilhomem, @davicostalf ?
"react-dom": "18.x" | ||
}, | ||
"devDependencies": { | ||
"@types/lodash": "^4.17.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@types/lodash": "^4.17.4", |
"dependencies": { | ||
"@vtex/shoreline-utils": "workspace:*", | ||
"echarts-for-react": "^3.0.2", | ||
"lodash": "^4.17.21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not using it, right?
"lodash": "^4.17.21", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i'll remove it
packages/charts/package.json
Outdated
"@vtex/shoreline-utils": "workspace:*", | ||
"echarts-for-react": "^3.0.2", | ||
"lodash": "^4.17.21", | ||
"vitest": "^1.6.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vistest is already on the global scope.
"vitest": "^1.6.0", |
packages/charts/src/chart.tsx
Outdated
* Render a Shoreline Chart with echarts | ||
* @see https://echarts.apache.org/en/index.html | ||
*/ | ||
export const Chart = forwardRef<echarts.EChartsType | undefined, ChartProps>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was facing a type error with ref, on the useImperativeHandle
and tried the undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this echarts.ECartsType return? Some HTMLDivElement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns the Chart instance, the same as in chartRef.current.getEchartsInstance()
packages/charts/src/chart.tsx
Outdated
useEffect(() => { | ||
window.addEventListener('resize', handleResize) | ||
return () => { | ||
window.removeEventListener('resize', handleResize) | ||
} | ||
}, [handleResize]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a check for dom. It is useful for SSR. You can use the canUseDom
util from @vtex/shoreline utils
useEffect(() => { | |
window.addEventListener('resize', handleResize) | |
return () => { | |
window.removeEventListener('resize', handleResize) | |
} | |
}, [handleResize]) | |
useEffect(() => { | |
if(!canUseDom) return | |
window.addEventListener('resize', handleResize) | |
return () => { | |
window.removeEventListener('resize', handleResize) | |
} | |
}, [handleResize]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tip!
An API question: What will happen to the |
After implement the variant this case come to my mind, at this moment, just bar has the horizontal variant, the first solution that i thought was when the type hasnt the variant, return the default. Second solution is create the variant type according to the type variants, like |
I moved the |
Updating the status, @beatrizmilhomem and @davicostalf are checking the design part of the PR. |
…/charts-package
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Summary
The
@vtex/shoreline-charts
library wrapsecharts
in a react component with consistent theming.Default styles
@vtex/shoreline-charts
will contains default styles for all chart, in order to get a easy setup for a chart, in this PR we've a demo with theBar
chart.For the default styles we have
variants
, which is a pre-defined attributes on theme in addition to a pre-defined values to the series itself, but the user is free to edit and customize the chart as your own.Installation
Examples
Simple chart setup
Variant chart