Legend position and offset#7564
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughLegend now supports Cartesian ChangesLegend Cartesian positioning
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LegendImpl
participant ReduxSelectors
participant getCartesianPosition
participant ChartOffset
participant LegendWrapper
LegendImpl->>ReduxSelectors: read legend area and chart view box
LegendImpl->>getCartesianPosition: calculate position and anchors
LegendImpl->>ChartOffset: reserve space for outside placement
LegendImpl->>LegendWrapper: apply CSS translation and offset styles
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bundle ReportChanges will increase total bundle size by 34.2kB (0.61%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sunburstAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sankeyAssets Changed:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (8)
src/component/TooltipBoundingBox.tsx (1)
97-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
CSSPropertiesconsistently.Since
CSSPropertieswas imported fromreactand used in the props definition, consider updatingpositionStyleandouterStyleto use it instead ofReact.CSSPropertiesfor consistency.♻️ Proposed fix
- const positionStyle: React.CSSProperties = props.hasPortalFromProps + const positionStyle: CSSProperties = props.hasPortalFromProps ? {} : { transition: resolveTransitionProperty({ prefersReducedMotion, isAnimationActive: props.isAnimationActive, active: props.active, animationDuration: props.animationDuration, animationEasing: props.animationEasing, }), ...cssProperties, pointerEvents: 'none', position: 'absolute', top: 0, left: 0, }; - const outerStyle: React.CSSProperties = { + const outerStyle: CSSProperties = { ...positionStyle, visibility: !state.dismissed && props.active && props.hasPayload ? 'visible' : 'hidden', ...props.wrapperStyle, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/component/TooltipBoundingBox.tsx` around lines 97 - 117, Update the positionStyle and outerStyle declarations in TooltipBoundingBox to use the imported CSSProperties type directly instead of React.CSSProperties, matching the existing props typing convention.www/src/views/IndexView/IndexLineChart.tsx (1)
74-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse CSS variables to support theme changes.
The surrounding
LineChartheavily uses CSS variables (e.g.,var(--color-border-3)andvar(--color-surface-base)) to seamlessly support theming, such as dark mode toggles on the landing page. Hardcodingblackandwhitein thewrapperStylewill cause visual inconsistencies when the theme changes. Consider updating theborderandbackgroundproperties to utilize these existing CSS variables.🎨 Proposed fix
<Legend position="insideTopRight" offset={20} - wrapperStyle={{ border: '1px solid black', borderRadius: 5, padding: '1ex', background: 'white' }} + wrapperStyle={{ border: '1px solid var(--color-border-3)', borderRadius: 5, padding: '1ex', background: 'var(--color-surface-base)' }} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@www/src/views/IndexView/IndexLineChart.tsx` around lines 74 - 78, Update the Legend wrapperStyle in IndexLineChart to replace hardcoded black and white border/background values with the existing theme CSS variables used by the surrounding LineChart, preserving the current border width, radius, padding, and positioning.src/component/Legend.tsx (2)
299-308: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid unconditionally calling
getCartesianPosition.
getCartesianPositionis called on every render, even whenprops.positionisundefined(which uses the legacy positioning flow). Consider calling it conditionally to avoid unnecessary computation and to guard against unexpected behavior when the position isn't provided.♻️ Proposed refactor
- const positionResult = getCartesianPosition({ - /* - * When calculating the position we use two different view boxes. - * Inside positions use the plot area; outside positions use the margin-inset - * chart area, placing the Legend beyond any axes. - */ - viewBox: positionViewBox ?? { x: 0, y: 0, width: chartWidth, height: chartHeight }, - position: props.position, - offset: props.offset ?? 0, - }); + const positionResult = props.position + ? getCartesianPosition({ + /* + * When calculating the position we use two different view boxes. + * Inside positions use the plot area; outside positions use the margin-inset + * chart area, placing the Legend beyond any axes. + */ + viewBox: positionViewBox ?? { x: 0, y: 0, width: chartWidth, height: chartHeight }, + position: props.position, + offset: props.offset ?? 0, + }) + : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/component/Legend.tsx` around lines 299 - 308, Update the position calculation around getCartesianPosition so it is invoked only when props.position is defined; preserve the legacy positioning flow when no position is provided and avoid passing an undefined position to the helper.
311-318: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSafe access to
positionResultfields.If you apply the suggested refactor to make
positionResultconditional, ensure that you also update thepositionStyleblock to safely access its fields.♻️ Proposed refactor
- const positionStyle: React.CSSProperties | undefined = props.position + const positionStyle: React.CSSProperties | undefined = props.position && positionResult ? { width: 'fit-content', height: 'fit-content', top: positionResult.y + (outsidePositionOffset.top ?? 0), left: positionResult.x + (outsidePositionOffset.left ?? 0), transform: cartesianPositionToCSSTranslate(positionResult.horizontalAnchor, positionResult.verticalAnchor), } : getDefaultPosition(wrapperStyle, props, margin, chartWidth, chartHeight, lastBoundingBox);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/component/Legend.tsx` around lines 311 - 318, Update the positionStyle construction in Legend so it only reads positionResult.y, positionResult.x, horizontalAnchor, and verticalAnchor when positionResult exists; preserve the current offset and transform behavior for the available result.src/cartesian/getCartesianPosition.tsx (1)
291-300: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid array allocation on every call.
Since this helper may be called frequently during rendering and layout phases, consider avoiding the array allocation on each call by either extracting the array to a module-level constant or using direct string comparisons.
♻️ Proposed refactor (direct comparisons)
export function isOutsidePosition(position: CartesianPosition | undefined): boolean { if (position == null) { return false; } if (typeof position === 'object') { return true; } - const absolutePositions = ['top', 'left', 'right', 'bottom']; - return absolutePositions.includes(position); + return position === 'top' || position === 'left' || position === 'right' || position === 'bottom'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cartesian/getCartesianPosition.tsx` around lines 291 - 300, Update isOutsidePosition to avoid creating the absolutePositions array on every invocation; use direct comparisons or a module-level constant for the allowed position values while preserving the current null, object, and string behavior.test-vr/tests/LegendPositionExample.spec-vr.tsx (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
.tsxextension from the import path.While Playwright's bundler might resolve this correctly, explicitly including
.tsxin import paths is an anti-pattern in standard TypeScript module resolution. Consider removing it for consistency with typical TS import practices.💡 Proposed change
-import LegendPositionExample from '../../www/src/docs/exampleComponents/Legend/LegendPositionExample.tsx'; +import LegendPositionExample from '../../www/src/docs/exampleComponents/Legend/LegendPositionExample';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-vr/tests/LegendPositionExample.spec-vr.tsx` at line 3, Update the LegendPositionExample import in LegendPositionExample.spec-vr.tsx to omit the explicit .tsx extension, preserving the same module target and import behavior.test/component/Legend/Legend.spec.tsx (2)
1875-1875: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out code or add a TODO.
These assertions are commented out, likely because they break across React versions (as noted in adjacent comments). Dead code should ideally be removed or accompanied by aTODO:comment so it can be actively tracked for future resolution.
test/component/Legend/Legend.spec.tsx#L1875-L1875: remove the commented-out assertion or add a TODO for the offset spy call count.test/component/Legend/Legend.spec.tsx#L1904-L1904: remove the commented-out assertion or add a TODO.test/component/Legend/Legend.spec.tsx#L1933-L1933: remove the commented-out assertion or add a TODO.test/component/Legend/Legend.spec.tsx#L1962-L1962: remove the commented-out assertion or add a TODO.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/component/Legend/Legend.spec.tsx` at line 1875, Remove the commented-out offset spy assertions at test/component/Legend/Legend.spec.tsx lines 1875, 1904, 1933, and 1962, or add a clear TODO comment at each location explaining the React-version compatibility issue and future resolution needed.
538-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClean up conversational scratchpad comments.
These comments read like draft notes or manual calculation traces (e.g., "If input y is 0 (from chart dimensions?), wait."). Consider removing them or condensing them into brief explanations of why the styles are expected.
test/component/Legend/Legend.spec.tsx#L538-L556: condense or remove the manual calculation trace forposition="top".test/component/Legend/Legend.spec.tsx#L594-L599: condense or remove the manual calculation trace forposition="insideBottomRight".test/component/Legend/Legend.spec.tsx#L635-L638: condense or remove the manual calculation trace forposition="left".test/component/Legend/Legend.spec.tsx#L696-L700: condense or remove the trace for coordinate object positioning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/component/Legend/Legend.spec.tsx` around lines 538 - 556, Clean up the scratchpad-style calculation comments in test/component/Legend/Legend.spec.tsx at lines 538-556, 594-599, 635-638, and 696-700. Remove conversational notes and manual traces, or condense each section to a brief explanation of the expected positioning styles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/cartesian/getCartesianPosition.tsx`:
- Around line 291-300: Update isOutsidePosition to avoid creating the
absolutePositions array on every invocation; use direct comparisons or a
module-level constant for the allowed position values while preserving the
current null, object, and string behavior.
In `@src/component/Legend.tsx`:
- Around line 299-308: Update the position calculation around
getCartesianPosition so it is invoked only when props.position is defined;
preserve the legacy positioning flow when no position is provided and avoid
passing an undefined position to the helper.
- Around line 311-318: Update the positionStyle construction in Legend so it
only reads positionResult.y, positionResult.x, horizontalAnchor, and
verticalAnchor when positionResult exists; preserve the current offset and
transform behavior for the available result.
In `@src/component/TooltipBoundingBox.tsx`:
- Around line 97-117: Update the positionStyle and outerStyle declarations in
TooltipBoundingBox to use the imported CSSProperties type directly instead of
React.CSSProperties, matching the existing props typing convention.
In `@test-vr/tests/LegendPositionExample.spec-vr.tsx`:
- Line 3: Update the LegendPositionExample import in
LegendPositionExample.spec-vr.tsx to omit the explicit .tsx extension,
preserving the same module target and import behavior.
In `@test/component/Legend/Legend.spec.tsx`:
- Line 1875: Remove the commented-out offset spy assertions at
test/component/Legend/Legend.spec.tsx lines 1875, 1904, 1933, and 1962, or add a
clear TODO comment at each location explaining the React-version compatibility
issue and future resolution needed.
- Around line 538-556: Clean up the scratchpad-style calculation comments in
test/component/Legend/Legend.spec.tsx at lines 538-556, 594-599, 635-638, and
696-700. Remove conversational notes and manual traces, or condense each section
to a brief explanation of the expected positioning styles.
In `@www/src/views/IndexView/IndexLineChart.tsx`:
- Around line 74-78: Update the Legend wrapperStyle in IndexLineChart to replace
hardcoded black and white border/background values with the existing theme CSS
variables used by the surrounding LineChart, preserving the current border
width, radius, padding, and positioning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47dfad44-eb08-4704-8083-41b0aaa71616
⛔ Files ignored due to path filters (53)
test-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-align-without-offset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-align-without-offset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-align-without-offset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---bottom-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---bottom-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---bottom-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---center-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---center-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---center-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottom-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottom-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottom-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomLeft-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomLeft-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomLeft-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTop-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTop-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTop-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopLeft-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopLeft-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopLeft-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---top-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---top-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---top-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/Customize.spec-vr.tsx-snapshots/CustomizeLegendAndTooltipStyle-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/Customize.spec-vr.tsx-snapshots/CustomizeLegendAndTooltipStyle-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/IndexView.spec-vr.tsx-snapshots/Index-Line-Chart-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/IndexView.spec-vr.tsx-snapshots/Index-Line-Chart-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/IndexView.spec-vr.tsx-snapshots/Index-Line-Chart-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (36)
omnidoc/commentSimilarityExceptions.tsscripts/snapshots/es6Files.txtscripts/snapshots/libFiles.txtscripts/snapshots/typesFiles.txtsrc/cartesian/XAxis.tsxsrc/cartesian/cartesianPositionToCSSTranslate.tsxsrc/cartesian/cartesianViewBoxToTrapezoid.tsxsrc/cartesian/getCartesianPosition.tsxsrc/component/DefaultLegendContent.tsxsrc/component/Label.tsxsrc/component/Legend.tsxsrc/component/TooltipBoundingBox.tsxsrc/context/chartLayoutContext.tsxsrc/index.tssrc/state/legendSlice.tssrc/state/selectors/legendSelectors.tssrc/state/selectors/selectChartOffsetInternal.tssrc/state/selectors/selectLegendArea.tssrc/util/ChartUtils.tssrc/util/tooltip/translate.tstest-vr/tests/LegendPosition.spec-vr.tsxtest-vr/tests/LegendPositionComponents.tsxtest-vr/tests/LegendPositionExample.spec-vr.tsxtest/cartesian/cartesianPositionToCSSTranslate.spec.tstest/cartesian/getCartesianPosition.spec.tstest/component/Legend/Legend.itemSorter.spec.tsxtest/component/Legend/Legend.spec.tsxtest/component/Legend/Legend.typed.spec.tsxtest/component/Tooltip/Tooltip.visibility.spec.tsxtest/state/selectors/legendSelectors.spec.tsxtest/state/selectors/selectChartOffset.spec.tsxtest/util/ChartUtils/appendOffsetOfLegend.spec.tsxwww/src/docs/exampleComponents/Legend/LegendPositionExample.tsxwww/src/docs/exampleComponents/Legend/index.tswww/src/docs/exampleComponents/Legend/index.tsxwww/src/views/IndexView/IndexLineChart.tsx
💤 Files with no reviewable changes (2)
- www/src/docs/exampleComponents/Legend/index.ts
- src/context/chartLayoutContext.tsx
|
Tip All tests passed and all changes approved!🟢 UI Tests: 198 tests unchanged |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7564 +/- ##
==========================================
+ Coverage 88.20% 88.21% +0.01%
==========================================
Files 615 619 +4
Lines 14272 14369 +97
Branches 3588 3638 +50
==========================================
+ Hits 12588 12676 +88
- Misses 1494 1499 +5
- Partials 190 194 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/util/ChartUtils.ts (1)
101-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the comment regarding position-based legends moving the plot area.
The comment on line 101 states that position-based legends "must not move the plot area", but the very next block (
if (isOutsidePosition(position))) modifies the chart's offsets to do exactly that.Consider updating the comment to clarify that this restriction applies primarily to inside positions, while outside positions still require offset adjustments to make room for the legend.
📝 Proposed comment update
- // Position-based legends are absolutely placed and must not move the plot area. + // Position-based legends are absolutely placed. They only move the plot area if they are positioned outside. if (isOutsidePosition(position)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/ChartUtils.ts` around lines 101 - 102, Update the comment immediately above the isOutsidePosition(position) check to clarify that inside-position legends do not move the plot area, while outside-position legends adjust chart offsets to reserve space.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/util/ChartUtils.ts`:
- Around line 101-102: Update the comment immediately above the
isOutsidePosition(position) check to clarify that inside-position legends do not
move the plot area, while outside-position legends adjust chart offsets to
reserve space.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a53f513-7bcd-4442-9a82-30a79fb6d25c
⛔ Files ignored due to path filters (33)
test-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-custom-offset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/Legend-position-with-default-offset-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-bottom-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-bottom-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-bottom-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-insideRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-insideRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-insideRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-left-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-left-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPosition.spec-vr.tsx-snapshots/VeryLongLegendText-with-position-left-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideBottomRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideLeft-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---insideTopRight-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---left-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/LegendPositionExample.spec-vr.tsx-snapshots/LegendPositionExample---right-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (13)
omnidoc/commentSimilarityExceptions.tsomnidoc/verifyExamples.spec.tssrc/cartesian/getCartesianPosition.tsxsrc/component/DefaultLegendContent.tsxsrc/component/Legend.tsxsrc/component/TooltipBoundingBox.tsxsrc/util/ChartUtils.tstest-vr/tests/LegendPosition.spec-vr.tsxtest-vr/tests/LegendPositionComponents.tsxtest/component/Legend/Legend.spec.tsxtest/util/ChartUtils/appendOffsetOfLegend.spec.tsxwww/src/docs/exampleComponents/Legend/LegendPositionExample.tsxwww/src/views/IndexView/IndexLineChart.tsx
💤 Files with no reviewable changes (1)
- omnidoc/verifyExamples.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- www/src/views/IndexView/IndexLineChart.tsx
- src/component/DefaultLegendContent.tsx
- src/component/TooltipBoundingBox.tsx
- test/component/Legend/Legend.spec.tsx
- src/component/Legend.tsx
- src/cartesian/getCartesianPosition.tsx
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. |
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. |
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Instead of old align + verticalAlign props, Legend now has the same
positionandoffsetprops as Label does. And applies different defaults based on the position too. This should make it more convenient and intuitive to customize the Legend.Motivation and Context
Easier to use API
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Legendpositioning withposition(including inside placements) and coordinate-based placement.Legendoffset, with improved defaultlayoutauto behavior.CartesianPositionfor easier typing.Bug Fixes
Documentation / Tests
Legend.position(including long-text scenarios).