Skip to content

Conversation

tewarig
Copy link
Contributor

@tewarig tewarig commented Sep 1, 2025

Description

Changes

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

@@ -0,0 +1 @@
export { ChartBarWrapper, ChartBar } from './BarCharts';
Copy link
Member

Choose a reason for hiding this comment

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

types are not exported from here

componentName="LineChart"
componentDescription="A Line Chart component built on top of Recharts with Blade design system styling."
figmaURL="https://www.figma.com/design/jubmQL9Z8V7881ayUD95ps/Blade-DSL?node-id=92678-188716&p=f&m=dev"
figmaURL="https://github.com/razorpay/blade/blob/master/packages/blade/src/components/Charts/_decisions/decisions.md"
Copy link
Member

Choose a reason for hiding this comment

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

wrong url

/**
* The layout of the bar chart.
*/
layout?: 'horizontal' | 'vertical';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
layout?: 'horizontal' | 'vertical';
orientation?: 'horizontal' | 'vertical';

we call it orientation in other components right 🤔

Copy link
Member

Choose a reason for hiding this comment

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

can you fix the file naming? "AreaChart" but "BarCharts" 🙈

* Chart data to be rendered
*/
data: data[];
} & Partial<Omit<BaseBoxProps, keyof FlexboxProps | keyof GridProps>>;
Copy link
Member

Choose a reason for hiding this comment

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

did you mean to use StyledPropsBlade here? why does it support type like this?

Copy link
Contributor Author

@tewarig tewarig Sep 23, 2025

Choose a reason for hiding this comment

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

if you want to give margin padding to chartWrapper it should support that, right ?
even we should be able to define width of the container in chartWrapper it self

Copy link
Member

Choose a reason for hiding this comment

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

Lets do & BoxProps then? its fine if flex and grid is supported as well. We can avoid creating too many different combinations of styled props on different components

export const VerticalBarChart: StoryFn<typeof ChartBar> = () => {
return (
<div style={{ width: '100%', height: '500px' }}>
<ChartBarWrapper data={chartData.slice(0, 5)} layout="vertical">
Copy link
Member

Choose a reason for hiding this comment

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

isn't this horizontal and the default one is vertical?

Copy link
Contributor Author

@tewarig tewarig Sep 23, 2025

Choose a reason for hiding this comment

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

you that's true. recharts follow the same nomenclature. so i have not changed it. (plus layout is related to chartwrapper not chart. i think that's why it is named that way.)
https://recharts.org/en-US/storybook

Comment on lines 295 to 298
export const BarChartWithInformationalColorTheme: StoryFn<typeof ChartBar> = () => {
return (
<div style={{ width: '100%', height: '500px' }}>
<ChartBarWrapper data={chartData.slice(0, 5)} layout="vertical" colorTheme="default">
Copy link
Member

Choose a reason for hiding this comment

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

The story name says its Informational Color Theme but the prop says its colorTheme="default". Is the default color theme informational color theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it to default color theme

const ChartBarWrapper: React.FC<ChartBarWrapperProps & TestID & DataAnalyticsAttribute> = ({
children,
colorTheme = 'default',
layout = 'horizontal',
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure the default layout is horizontal?

Wouldn't this chart be a vertical chart and not horizontal chart?

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 the layout is horizontal , but the bar charts are vertical.

Screenshot 2025-09-23 at 12 29 24 PM

Copy link
Member

Choose a reason for hiding this comment

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

Screen.Recording.2025-09-23.at.10.24.57.AM.mov

Can you check with RK if we're expected to not have any transition when we hover over the chart items? right now it feels too sudden

Copy link
Contributor Author

@tewarig tewarig Sep 23, 2025

Choose a reason for hiding this comment

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

i did try to add custom transition with css classes but is not working.
so rechart render entire child's so you see sudden opacity change in rechart colors. i am trying to find work around of this.

the main issue is we use rect to show the white bar between bars in barGraph. which does not allow us to have animated properties for fill 😢

Screen.Recording.2025-09-23.at.5.05.45.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol was able to find a workaround for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried using classes but that was working so.
i used framer motion.

Copy link
Member

Choose a reason for hiding this comment

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

Lets skip it for now if its too hacky and complex

Copy link
Member

@saurabhdaware saurabhdaware left a comment

Choose a reason for hiding this comment

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

added some minor comments. rest looks good

* The orientation of the bar chart.
*/
layout?: 'horizontal' | 'vertical';
orientation?: 'horizontal' | 'vertical';
Copy link
Member

Choose a reason for hiding this comment

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

we can go back to layout like we discussed in meeting since orientation and layout are 2 different things

@@ -1,5 +1,5 @@
import React from 'react';
import { ChartBarWrapper, ChartBar } from '../BarCharts';
import { ChartBarWrapper, ChartBar } from '../BarChart.web';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { ChartBarWrapper, ChartBar } from '../BarChart.web';
import { ChartBarWrapper, ChartBar } from '../BarChart';

Copy link
Member

Choose a reason for hiding this comment

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

Lets skip it for now if its too hacky and complex

const MotionRectangle = (props: MotionRectProps): React.ReactElement => {
const { fillOpacity, barIndex, initialOpacity, ...restProps } = props;
return (
<LazyMotion features={domAnimation}>
Copy link
Member

Choose a reason for hiding this comment

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

LazyMotion is added by consumers in their applications. We shouldn't add it in components

const isVertical = layout === 'vertical';
const isVertical = orientation === 'vertical';

const getInitialOpacity = (): number => {
Copy link
Member

Choose a reason for hiding this comment

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

Lets skip it for now maybe? code seems too complex for just 1 transition. We'll figure out later

* Chart data to be rendered
*/
data: data[];
} & Partial<Omit<BaseBoxProps, keyof FlexboxProps | keyof GridProps>>;
Copy link
Member

Choose a reason for hiding this comment

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

Lets do & BoxProps then? its fine if flex and grid is supported as well. We can avoid creating too many different combinations of styled props on different components

@tewarig tewarig merged commit 1b07633 into master Sep 24, 2025
19 of 22 checks passed
@tewarig tewarig deleted the feat/add-bar-charts branch September 24, 2025 10:46
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.

4 participants