Add axis side placement for right-to-left locales - #98
Open
3li7alaki wants to merge 2 commits into
Open
Conversation
Resolve each axis to a plot edge and an outward sign so `axis.side` can move the line, stubs, tick labels, title, and crosshair value label to the opposite edge. Automatic margins follow the placement, and an unset side keeps the existing scene output.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThe chart API adds ChangesAxis-side placement
Benchmark metadata refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change adds optional axis-side placement while preserving existing defaults, with targeted coverage for axes, margins, crosshair labels, and rendering. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ChartConfig
participant scene
participant axisPlacement
participant crosshairResolver
ChartConfig->>scene: set x.axis.side and y.axis.side
scene->>axisPlacement: resolve axis edges and direction signs
axisPlacement-->>scene: return placement data
scene->>scene: render axes, labels, titles, guides, and margins
crosshairResolver->>axisPlacement: resolve guide-side placement
axisPlacement-->>crosshairResolver: return edge and direction
crosshairResolver-->>scene: position crosshair value labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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.
Closes #87.
Why
Arabic, Hebrew, and Farsi read right to left, and in those locales the value
axis belongs on the right. That is not a stylistic preference: the reading eye
starts at the right edge, so a left-hand value axis puts the scale behind the
data instead of in front of it.
@tanstack/chartshad no lever for it.ChartAxisPresentationOptionscoveredthe line, ticks, tick labels, and title, but never the placement, so the y axis
always resolved against the left plot edge.
x.reversealready orderedcategories right to left, which made the axis the single remaining element
pointing the wrong way.
The usual workarounds are not workarounds.
transform: scaleX(-1)on thecontainer mirrors the marks and the tick text and breaks pointer hit-testing.
direction: rtlon the host changes nothing, because placement is resolvedduring layout rather than by the browser.
We hit this migrating a bilingual production dashboard off Recharts, which
covers the case with
<YAxis orientation="right" />. Everything else about themigration was an improvement, so we shipped the Arabic locale with a knowingly
misplaced axis. This patch is the fix for that, written so the same option
serves anyone who wants a right-hand or top axis in any locale.
@Thom-ASM pointed at
scene.tsin the issue thread, which was the right placeto start.
API
ChartAxisSideis'start' | 'end'rather than physical literals. The y axisreads
startas left andendas right; the x axis readsstartas bottom andendas top. One spelling covers both dimensions, composes with the existingreverse, and keeps a right-to-left chart to a single flag per axis rather thana per-axis vocabulary the caller has to memorize.
sidedefaults tostart, so every existing chart is untouched.Implementation
axisPlacement()resolves each axis once into a plot edge and an outward sign,and every coordinate derives from that pair: the axis line, the tick stubs, the
tick labels and their default anchor, the title with its rotation, and the
crosshair value label. No site branches on the side on its own.
Two things fell out of the existing design rather than needing new code:
gutter moves to the other edge on its own.
all twelve packages follow without a change.
The crosshair is included because it is the other half of a placed axis. Its
value labels are resolved outside
createAxes, soSceneFocusGuideAxiscarriesthe side and the resolver derives the same placement from the guide's own plot
bounds.
Compatibility
sideis optional and defaults to the current behavior. A test asserts that anexplicit
'start'produces a scene node tree identical to an unset side, sothis is provably inert for existing charts rather than only intended to be.
Verification
charts-coretests pass, the 897 existing ones unchanged.stubs, label anchor, 90 degree title, and the margin moving from left to
right), the end-side x axis against the top edge, and the start-side
equivalence above.
included.
sourceHashchanges, which is a second check that default rendering did notmove.
with Eastern Arabic numerals: default,
y.side: 'end'withx.reverse, andx.side: 'end'.Changeset, root docs, and an
API-FRICTION.mdentry (F-285) are included perCONTRIBUTING.mdandAGENTS.md. Package versions and changelogs are left tothe automated version pull request.
Two notes on the baselines
The comparison bundle baseline is refreshed in its own commit, following the
existing convention. Competitor measurements reproduced byte for byte, and only
the TanStack entries, the input digest, and the timestamp move.
benchmarks/bundle-size/universal-baseline.jsonis deliberately notrefreshed. It already mismatches on unmodified
mainin my environment, so Icould not separate a real delta from local drift and did not want to lock in
numbers measured on my laptop. For the record, the placement code adds roughly
190 B gzip to the React adapter here. Say the word and I will include the
refresh, or leave it to a maintainer run.