-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Split bar graph into two distinct horizontal and vertical bars #15061
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
base: main
Are you sure you want to change the base?
Conversation
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.
Greptile Overview
Greptile Summary
This PR implements a comprehensive refactoring that splits the generic BAR chart type into two distinct chart types: VERTICAL_BAR and HORIZONTAL_BAR. The changes affect the entire chart system architecture, from the GraphQL schema to frontend components and test files.
Key changes include:
- GraphType enum update:
GraphType.BAR
is replaced withGraphType.VERTICAL_BAR
andGraphType.HORIZONTAL_BAR
- Field naming standardization: Axis-specific field names (
groupByFieldMetadataIdX/Y
,orderByX/Y
) are renamed to semantic primary/secondary axis names (primaryAxisGroup/secondaryAxisGroup
,primaryAxisOrderBy/secondaryAxisOrderBy
) - New icon component:
IconChartBarHorizontal
created by rotating the existing bar chart icon 90 degrees - Configuration DTOs updated: Both bar and line chart configurations now use the unified primary/secondary axis terminology
- Component updates: Chart selection, dropdown components, and rendering logic updated to handle both orientations
- Enhanced data transformation: Chart data transformation functions now include layout information and axis field mapping utilities
The refactoring maintains the same underlying configuration structure for both bar chart types while enabling distinct visual representations and user selection options. Integration tests, Storybook stories, and GraphQL fragments are comprehensively updated to reflect these changes.
Important Files Changed
Changed Files
Filename | Score | Overview |
---|---|---|
packages/twenty-server/src/engine/core-modules/page-layout/enums/graph-type.enum.ts | 4/5 | Splits generic BAR enum into VERTICAL_BAR and HORIZONTAL_BAR types |
packages/twenty-server/src/engine/core-modules/page-layout/dtos/bar-chart-configuration.dto.ts | 4/5 | Updates DTO to support both bar chart types with primary/secondary axis naming |
packages/twenty-server/src/engine/core-modules/page-layout/dtos/line-chart-configuration.dto.ts | 4/5 | Renames X/Y axis fields to primary/secondary axis terminology |
packages/twenty-front/src/generated/graphql.ts | 4/5 | Major GraphQL schema changes for chart configurations and GraphType enum |
packages/twenty-front/src/modules/command-menu/pages/page-layout/constants/GraphTypeInformation.ts | 4/5 | Splits BAR entry into separate vertical and horizontal bar configurations |
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/getBarChartSettings.ts | 4/5 | Converts static settings to dynamic function handling both bar orientations |
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/getBarChartAxisFields.ts | 4/5 | New utility for mapping chart fields to X/Y axes based on orientation |
packages/twenty-front/src/modules/page-layout/widgets/graph/utils/transformGroupByDataToBarChartData.ts | 4/5 | Adds layout support and updates field references for axis mapping |
packages/twenty-front/src/modules/page-layout/widgets/graph/components/GraphWidget.tsx | 5/5 | Simple switch statement update to handle both bar chart types |
packages/twenty-front/src/modules/command-menu/pages/page-layout/components/ChartTypeSelectionSection.tsx | 4/5 | Updates chart type options to include both vertical and horizontal bars |
packages/twenty-ui/src/display/icon/components/IconChartBarHorizontal.tsx | 4/5 | New icon component created by rotating existing bar chart icon 90 degrees |
packages/twenty-front/src/modules/page-layout/utils/createDefaultGraphWidget.ts | 4/5 | Adds HORIZONTAL_BAR case and updates property naming convention |
packages/twenty-front/src/modules/page-layout/widgets/graph/graphWidgetBarChart/components/GraphWidgetBarChartRenderer.tsx | 5/5 | Adds layout prop extraction and passing to chart component |
packages/twenty-front/src/modules/page-layout/widgets/graph/graphWidgetBarChart/hooks/useGraphBarChartWidgetData.ts | 4/5 | Adds optional layout property to hook return type |
packages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts | 4/5 | Refactors to use chart-type-specific field mapping logic |
Confidence score: 4/5
- This PR represents a significant architectural change that requires careful coordination between frontend and backend, but the implementation appears well-structured and comprehensive
- Score reflects the breaking nature of GraphQL schema changes and the complexity of the refactoring, though the changes are consistently applied across the codebase
- Pay close attention to the GraphQL schema changes in generated/graphql.ts and DTO files, as these represent breaking changes that will affect API consumers
Sequence Diagram
sequenceDiagram
participant User
participant ChartTypeSelectionSection as "ChartTypeSelectionSection"
participant MenuPicker
participant GraphTypeInformation as "GRAPH_TYPE_INFORMATION"
User->>ChartTypeSelectionSection: "Selects chart type"
ChartTypeSelectionSection->>GraphTypeInformation: "Get icon and label for GraphType"
GraphTypeInformation-->>ChartTypeSelectionSection: "Return icon, label"
ChartTypeSelectionSection->>MenuPicker: "Render option with icon/label"
MenuPicker->>ChartTypeSelectionSection: "onClick callback"
ChartTypeSelectionSection->>User: "setCurrentGraphType(graphType)"
Additional Comments (1)
-
packages/twenty-front/src/modules/page-layout/graphql/fragments/pageLayoutWidgetFragment.ts
, line 19-56 (link)style: The BarChartConfiguration and LineChartConfiguration fragments now have identical field structures. Consider extracting these shared fields into a reusable fragment to reduce duplication and improve maintainability.
Context used:
- Context from
dashboard
- Use 'type' instead of 'interface' for type definitions in the project. (source) - Context from
dashboard
- Avoid creating draft pull requests; they are not preferred in our workflow. (source)
44 files reviewed, 16 comments
packages/twenty-ui/src/display/icon/components/IconChartBarHorizontal.tsx
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/page-layout/enums/graph-type.enum.ts
Show resolved
Hide resolved
case GraphType.HORIZONTAL_BAR: | ||
return { | ||
items: [ | ||
{ category: 'Jan', value: 45 }, | ||
{ category: 'Feb', value: 52 }, | ||
{ category: 'Mar', value: 48 }, | ||
{ category: 'Apr', value: 61 }, | ||
{ category: 'May', value: 55 }, | ||
], | ||
indexBy: 'category', | ||
keys: ['value'], | ||
seriesLabels: { value: 'Value' }, | ||
layout: 'horizontal' as const, | ||
}; |
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.
style: The HORIZONTAL_BAR case uses identical sample data to VERTICAL_BAR. Consider using different sample data to better demonstrate horizontal layout characteristics.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/page-layout/utils/getDefaultWidgetData.ts
Line: 44:57
Comment:
**style:** The HORIZONTAL_BAR case uses identical sample data to VERTICAL_BAR. Consider using different sample data to better demonstrate horizontal layout characteristics.
How can I resolve this? If you propose a fix, please make it concise.
case GraphType.VERTICAL_BAR: | ||
case GraphType.HORIZONTAL_BAR: | ||
return { w: 6, h: 6 }; |
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.
style: Both bar chart types use the same widget size. Consider if horizontal bar charts should have different dimensions for better visual representation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/page-layout/utils/getDefaultWidgetData.ts
Line: 153:155
Comment:
**style:** Both bar chart types use the same widget size. Consider if horizontal bar charts should have different dimensions for better visual representation.
How can I resolve this? If you propose a fix, please make it concise.
packages/twenty-front/src/modules/command-menu/pages/page-layout/utils/getBarChartAxisFields.ts
Show resolved
Hide resolved
...u/pages/page-layout/components/dropdown-content/ChartXAxisSortBySelectionDropdownContent.tsx
Outdated
Show resolved
Hide resolved
...u/pages/page-layout/components/dropdown-content/ChartXAxisSortBySelectionDropdownContent.tsx
Outdated
Show resolved
Hide resolved
...ages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts
Outdated
Show resolved
Hide resolved
...ages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts
Outdated
Show resolved
Hide resolved
...ages/twenty-front/src/modules/command-menu/pages/page-layout/hooks/useChartSettingsValues.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/page-layout/dtos/bar-chart-configuration.dto.ts
Show resolved
Hide resolved
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:51261 This environment will automatically shut down when the PR is closed or after 5 hours. |
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[log] [log] ✖ Field groupByFieldMetadataIdX was removed from object type BarChartConfiguration
GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[log] [log] ✖ Field groupByFieldMetadataIdX was removed from object type BarChartConfiguration
|
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.
Summary
Split BAR into VERTICAL_BAR and HORIZONTAL_BAR as separate chart types.
The Problem
Initially wanted a simple vertical/horizontal toggle for bar charts, but ran into a GraphQL union type
constraint: union types can't have the same field name with different nullability.
The Solution
Use primaryAxis and secondaryAxis naming instead of X/Y:
This way both chart types can share the same DTO structure with consistent nullability, and we map
primary/secondary to X/Y based on orientation at runtime.
What Changed
variants)
video QA
2025-10-13.18-31-00.mp4