feat: add barAlign prop for left/right/center bar alignment#6740
feat: add barAlign prop for left/right/center bar alignment#6740Harshit-jain-57 wants to merge 1 commit into
Conversation
- Add barAlign prop to BarChart (left, center, right) - Modify bar positioning logic to support alignment - Default to center for backward compatibility Fixes recharts#6296
WalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state/selectors/barSelectors.ts (1)
240-247: Right alignment is not honored when bar size comes frommaxBarSize(implicit bar size branch)In the branch where bar sizes are derived from
barCategoryGap/barGapand then capped bymaxBarSize, the offset term:offset: offset + (originalSize + realBarGap) * i + (barAlign === 'center' ? (originalSize - size) / 2 : 0),makes
'left'and'right'behave identically: both anchor bars at the left of their slot. As a result,barAlign="right"has no effect in this path, even though it works correctly in the explicitbarSizebranch.You can fix this by computing a single per‑slot inner offset that handles all three modes:
@@ function getBarPositions(…) - let originalSize = (bandSize - 2 * offset - (len - 1) * realBarGap) / len; + let originalSize = (bandSize - 2 * offset - (len - 1) * realBarGap) / len; @@ - const size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) : originalSize; - result = sizeList.reduce( + const size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) : originalSize; + const alignedOffsetWithinSlot = + barAlign === 'left' + ? 0 + : barAlign === 'right' + ? originalSize - size + : (originalSize - size) / 2; + + result = sizeList.reduce( (res: ReadonlyArray<BarWithPosition>, entry: BarCategory, i): ReadonlyArray<BarWithPosition> => [ ...res, { stackId: entry.stackId, dataKeys: entry.dataKeys, position: { - offset: offset + (originalSize + realBarGap) * i + (barAlign === 'center' ? (originalSize - size) / 2 : 0), + offset: offset + (originalSize + realBarGap) * i + alignedOffsetWithinSlot, size, }, }, ], initialValue, );This preserves the existing centered behavior for
'center', keeps'left'as left‑aligned, and correctly right‑aligns the bars within their slot whenbarAlign="right".Also applies to: 276-278, 304-322, 353-364
🧹 Nitpick comments (2)
src/state/selectors/rootPropsSelectors.ts (1)
8-8: Consider centralizing thebarAlignunion into a shared type
'left' | 'center' | 'right'is now duplicated acrossCartesianChartProps,UpdatableChartOptions, and this selector. Consider introducing aBarAligntype insrc/util/types.tsand reusing it here to keep the allowed values in sync and reduce maintenance overhead.src/util/types.ts (1)
1108-1116: Doc comment could mentionmaxBarSize-driven shrinking as wellThe new
barAlignJSDoc only mentions the case “whenbarSizeis specified”, but the positioning logic also uses it when bars are limited bymaxBarSize(with no explicitbarSize). You may want to clarify that in the comment so the documented behavior fully matches runtime behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/state/rootPropsSlice.ts(3 hunks)src/state/selectors/barSelectors.ts(7 hunks)src/state/selectors/rootPropsSelectors.ts(1 hunks)src/util/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run ESLint and Prettier on the codebase using
npm run lint
Files:
src/state/selectors/rootPropsSelectors.tssrc/state/rootPropsSlice.tssrc/util/types.tssrc/state/selectors/barSelectors.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run type checking on the codebase using
npm run check-types
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas const
Files:
src/state/selectors/rootPropsSelectors.tssrc/state/rootPropsSlice.tssrc/util/types.tssrc/state/selectors/barSelectors.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/state/selectors/rootPropsSelectors.tssrc/state/rootPropsSlice.tssrc/util/types.tssrc/state/selectors/barSelectors.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any strings or formatting choices in library code; users should provide localized strings as desired
Files:
src/state/selectors/rootPropsSelectors.tssrc/state/rootPropsSlice.tssrc/util/types.tssrc/state/selectors/barSelectors.ts
🧬 Code graph analysis (2)
src/state/selectors/rootPropsSelectors.ts (1)
src/state/store.ts (1)
RechartsRootState(23-38)
src/state/selectors/barSelectors.ts (1)
src/state/selectors/rootPropsSelectors.ts (1)
selectBarAlign(8-8)
🔇 Additional comments (2)
src/state/selectors/barSelectors.ts (1)
22-22: Unable to access the repository to verify selector wiring and argument order consistencyThe sandbox environment cannot clone the recharts/recharts repository needed to inspect the referenced lines (22, 353-364, 376-392) and verify the claims about
selectBarAlignimport,combineAllBarPositionsparameter wiring, and argument order matching. Additionally, the linting and type-checking commands cannot be executed in this environment.Please run
npm run lintandnpm run check-typeslocally to verify that the updated signatures, imports, and selector wiring compile cleanly and pass all checks.src/state/rootPropsSlice.ts (1)
14-14: Unable to complete verification due to repository access failure. Manual verification needed to confirm that allupdateOptionscall sites properly handle the newbarAlignfield.
| } | ||
|
|
||
| const offset = ((bandSize - sum) / 2) >> 0; | ||
| const offset = barAlign === 'left' ? 0 : barAlign === 'right' ? bandSize - sum : ((bandSize - sum) / 2) >> 0; |
ckifer
left a comment
There was a problem hiding this comment.
We need unit tests and probably a snapshot test.
Also need to update the docs
| accessibilityLayer: boolean; | ||
| barCategoryGap: number | string; | ||
| barGap: number | string; | ||
| barAlign: 'left' | 'center' | 'right'; |
There was a problem hiding this comment.
can you make this a defined type instead of typing out the union every time? (and use that type everywhere)
|
Can you also share a screenshot? |
|
Does this need to be internal? Can we achieve the same with https://d3js.org/d3-scale/band#band_align ? |
Fixes #6296
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.