Skip to content
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

Feat/stacked column chart react #78

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

IlyesBenAmara
Copy link
Contributor

@IlyesBenAmara IlyesBenAmara commented Oct 19, 2022

Created a new chart recipe called StackedColumnChart.
Note: if the review goes well, I will add the proper unit test for it and then continue my way on adding this recipe to the Vue package. (will probably use the same branch for the React unit tests)
image

Copy link
Contributor

@marrouchi marrouchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👋 Left some comments ;)

packages/ez-dev/storybook/data.ts Outdated Show resolved Hide resolved
packages/ez-react/src/components/StackedBars.tsx Outdated Show resolved Hide resolved
packages/ez-react/src/components/StackedBars.tsx Outdated Show resolved Hide resolved
packages/ez-react/src/components/StackedBars.tsx Outdated Show resolved Hide resolved
@marrouchi marrouchi force-pushed the feat/StackedColumnChart-react branch from 00b137f to 70cf3b5 Compare October 25, 2022 06:29
@@ -37,7 +62,15 @@ export const Bars: FC<BarsProps> = ({ xDomainKey, yDomainKey, ...rest }) => {
isRTL,
]);

return (
return scopedSlots && scopedSlots.default ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return scopedSlots && scopedSlots.default ? (
return <g className="ez-bars" {...rest}>CONDITION</g>

return (
<g {...rest}>
{yDomainKeys.map((yDomainKey) => {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove ?

const StackedColumnArguments = {
...defaultArguments,
yAxis: {
domainKeys: ['value1', 'value', 'value2'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
domainKeys: ['value1', 'value', 'value2'],
domainKeys: ['value', 'value1', 'value2'],

<ColorScale domain={yAxis.domainKeys} range={colors}>
{activeDomainKeys.map((activeDomaiKey) => {
return (
// we need to find a way to sort the the data columns so that it shows bars in the corret manner
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// we need to find a way to sort the the data columns so that it shows bars in the corret manner
// We need to find a way to sort the the data columns so that it shows bars in the corret manner

}}
>
<ColorScale domain={yAxis.domainKeys} range={colors}>
{activeDomainKeys.map((activeDomaiKey) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{activeDomainKeys.map((activeDomaiKey) => {
{activeDomainKeys.map((activeDomainKey) => {

// we need to find a way to sort the the data columns so that it shows bars in the corret manner
<Bars
xDomainKey={xAxis.domainKey}
yDomainKey={activeDomaiKey}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
yDomainKey={activeDomaiKey}
yDomainKey={activeDomainKey}

Comment on lines 113 to 114
shapeDatum.height >
previousShapeData[index].height
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shapeDatum.height >
previousShapeData[index].height
index > 0

@IlyesBenAmara IlyesBenAmara changed the base branch from master to feat/multiAreaChart-vue October 31, 2022 12:54
@IlyesBenAmara IlyesBenAmara changed the base branch from feat/multiAreaChart-vue to master October 31, 2022 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants