feat: add core RTL support for headers and column resizing - #1271
feat: add core RTL support for headers and column resizing#1271jahanbakhsh18 wants to merge 2 commits into
Conversation
|
Can you please provide a print screen of the result, so we can visually see the result without having to run it locally. @6pac the PR seems ok but asking Claude to double-check might be a good idea. SlickGrid/src/styles/_variables.scss Line 30 in bdadd27 SlickGrid/src/styles/_variables.scss Lines 45 to 46 in bdadd27 Any of the SASS variables can be used by simply replacing the body {
--alpine-cell-justify-content: flex-end;
} |
|
Regarding dirSide()
CSS alone cannot handle these behavioral changes. But I'm happy to explore removing
Let me know your thoughts on the path forward... |
|
Here is Claude Code's review (quite a lot!). Let me know if you want a suggested work plan from it. PR #1271 — core RTL support for headers and column resizingSummaryThe direction is welcome and the approach is refreshingly minimal: an opt-in flag that defaults to However, "core RTL support" currently overstates what works. Two supported code paths are broken once an RTL grid is wide enough to scroll horizontally, and both are invisible in the accompanying demo because the demo cannot scroll horizontally at all. The gap is not in what the PR changed — it is in the surface it did not reach. How this was verifiedRather than reading the diff alone, the branch was checked out, built ( The full Cypress suite was not run on this branch, so nothing here should be read as a broad regression claim — CI covers that. Blocking issues1.
|
scrollLeft after the call |
columns on screen | |
|---|---|---|
LTR (example1-simple) |
617 (scrolled) | 3, 4, 5 — target visible |
RTL (example1-simple-rtl) |
0 (no movement) | 0, 1, 2 — target off-screen |
The cause is the sign convention. In RTL, element.scrollLeft starts at 0 at the right edge and runs negative down to -(scrollWidth - clientWidth). internalScrollColumnIntoView — along with the rest of the horizontal machinery — assumes the LTR convention of a positive 0…max range, so its "is this column already visible?" test returns true for a column that is in fact off-screen, and no scroll is issued.
This is not an isolated call site. slick.grid.ts contains 51 references to scrollLeft, and this PR changes none of them. The same assumption shows up in cell virtualization: driving the viewport to the far end (scrollLeft = -617) leaves the visible area with zero cells rendered, because the computed leftPx/rightPx window lands in the wrong place and cleanUpAndRenderCells removes precisely the cells that are on screen.
Practically, this means an RTL grid works as long as every column fits, and degrades badly as soon as it does not.
2. Frozen columns overlap in RTL
Applying setOptions({ frozenColumn: 1 }) to the RTL demo produces panes that sit on top of one another:
frozen pane: left -412, width 400 -> spans -412 … -12
scrollable pane: left -210, width 198 -> spans -210 … -12 (entirely inside the frozen pane)
In LTR these two panes are adjacent and non-overlapping. Pane positioning is done with left offsets that were not mirrored, so in RTL the scrollable pane is drawn over the frozen one instead of beside it. In RTL the frozen pane should be on the right, with the scrollable pane extending to its left.
Fixing frozen panes may reasonably be out of scope for a first RTL PR — but in that case the combination should be explicitly rejected, or documented as unsupported, rather than silently rendering overlapping panes.
3. The demo cannot exercise either problem
example1-simple-rtl.html uses six columns at default widths — 80, 80, 90, 80, 80, 90 — totalling 500px inside a 583px viewport. Measured: scrollWidth === clientWidth, so the grid never scrolls horizontally.
This explains how "manual testing confirmed... no regression" could hold while issues 1 and 2 were present: the demo only exercises the case where everything fits. Widening the demo, or adding a few more columns so it genuinely scrolls, is the single highest-value change to make to this PR — it surfaces issue 1 immediately and makes the feature demonstrable rather than merely plausible.
Design suggestions
4. The !important override belongs in the library, not the example
The example carries this workaround:
.slick-resizable-handle { right: auto !important; left: 0 !important; }If every consumer has to hand-write !important CSS to get resize handles on the correct side, the library does not really support RTL — it tolerates it. This rule belongs in slick.grid.scss or the themes, scoped behind a slick-rtl class on the container (or [dir="rtl"] selectors), so that setting rtl: true is sufficient on its own.
5. The grid never sets direction on its own container
The demo relies on <html dir="rtl"> for the actual direction; the rtl option only drives the JavaScript-side positioning. That means an RTL grid embedded in an otherwise LTR page will not render correctly, which is the common real-world case — an admin screen in English containing one Arabic or Hebrew data table.
Setting direction: rtl (or an slick-rtl class) on the grid container from the option would make the feature self-contained.
6. CSS logical properties would remove most of the branching
inset-inline-start and inset-inline-end resolve to left/right according to the container's direction. Adopting them instead of left/right would collapse this work into a single code path: no dirSide getter, no if (this._options.rtl) branch in applyColumnWidths, and layout that follows direction automatically without a parallel set of RTL rules to maintain.
The current approach works, but it establishes a pattern where every future left/right site in the codebase needs the same manual mirroring, and each one is a place where LTR and RTL can drift apart. Browser support for logical properties is now universal, so this is worth weighing before the branching approach becomes entrenched.
7. Runtime toggling is only partly handled
createCssRules() is re-run by setColumns, so the .lN/.rN and .slick-header-column rules are correctly regenerated if rtl changes at runtime. But the inline style on the header containers — { [this.dirSide]: '-1000px' } on _headerL and _headerR — is written once during construction and never revisited, so setOptions({ rtl }) would leave a stale left: -1000px behind.
Either update those two elements when the option changes, or document rtl as a construction-time-only option so nobody expects otherwise.
8. Consider auto-detecting direction
getComputedStyle(container).direction === 'rtl' would let the grid pick up the ambient direction, with the explicit rtl option retained as an override. This removes the failure mode where the option and the DOM disagree — currently, setting rtl: true on a page that is not RTL, or vice versa, produces a half-mirrored grid with no warning.
Smaller items
- Getter type.
protected get dirSide()infersstring, not'left' | 'right'. Annotating the return type keeps the computed-key style object ({ [this.dirSide]: '-1000px' }) type-safe. - Charset.
example1-simple-rtl.htmldeclarescharset=iso-8859-1. For an RTL demo this must be UTF-8, or any Arabic, Persian or Hebrew content will mojibake. - Description vs. code. The PR description mentions "Persian sample data", but the committed data is English (
Task 0,5 days). It also says "AddedhideSidegetter", while the code hasdirSide. - Missing newline at the end of
example1-simple-rtl.html. - No automated test. The recent convention in this repo (fix: frozen-bottom grids never clean up off-screen row cells #1255, fix: footer-row lifecycle - construction crash, inconsistent getFooterRow, double onFooterRowCellRendered #1256, fix: fractional grid height leaves the bottom rows unrendered (#1262) #1263) is a Cypress spec landing in the same PR as the change; this one links an external
test-results.txtinstead. A spec asserting header/cell alignment andscrollCellIntoViewbehaviour under RTL would have caught issue 1, and would keep the feature from regressing as the surrounding code changes. - JSDoc scope. The
dirSidecomment describes only the "hide header columns off-screen" use, but the getter is also used for real header container positioning.
Checked and not an issue
The resize direction logic holds up. The handle sits on the column's visual left edge in RTL, which is still the boundary between columns i and i+1, so swapping the shrink/stretch leeways in the minPageX/maxPageX clamp and then negating d are complementary rather than double-correcting: physical movement is clamped in physical space, then converted to a logical delta.
Header and cell positions stayed in lockstep at every scroll position tested, in both directions — the header/body synchronisation is not affected by the scrollLeft sign problem described in issue 1.
Suggested path forward
- Widen the demo so it scrolls horizontally — this is what makes the remaining work visible and reviewable.
- Address the
scrollLeftsign convention, or scope the feature explicitly to non-scrolling grids for now. - Decide on frozen columns: mirror the panes, or reject the combination with a clear message.
- Move the resize-handle CSS into the themes.
- Add a Cypress spec covering alignment and
scrollCellIntoViewin RTL.
Points 1, 4 and 5 are small. Point 2 is the real work, and is worth deciding on before the branching pattern in point 6 above spreads any further.
|
Thank you for the incredibly thorough review, @6pac. I truly appreciate the time and deep testing you've put into this. I've read through the feedback and agree with the assessment. The Let me take some time to process the full review and I'll come back with a detailed work plan. I'll focus first on fixing the scroller issue and will provide clear updates soon. Thanks again for your time and guidance! |
|
@jahanbakhsh18 I'd love to take credit, but note that that was entirely done by Claude Code. I'm happy to get it to create work plan if you like, it costs almost nothing and saves us all a huge amount of time. |
|
I appreciate the offer! I use AI tools myself and know they can be incredibly helpful for certain tasks. |
|
@jahanbakhsh18 we're about to do a release though, so would it better to just merge what we have now and you can push some fix later? unless you can have it done by tomorrow, which should be about the time we push a release!? |
|
That's great news. I'd love to have this included in the upcoming release!
I can definitely have the scroller fix, updated demo, and RTL Cypress tests completed and pushed by the deadline. What time tomorrow do you need the commit by? I'll work backwards from that to make sure everything is ready. |
|
@jahanbakhsh18 I'll push after my working hours on Monday, around 7-8pm EST (Eastern TimeZone). I'm in Canada and @6pac (Ben) is in Australia, day and night 😆 Side note, I have my own SlickGrid repo and I've replicated your changes in mine too but I had to adjust a few things to get it all working. For example, I'm not sure if you tested with the Grid Menu, Header Menu, Column Picker, ... but in my repo, all these things are enabled by default and I have my own CSS styling themes as well, so a few CSS changes were required, there's a print screen of what it looks like in my repo and I didn't know that RTL really inverses everything, even column picker checkbox right instead of left, it feels a bit weird for us 😄 |
|
Thank you so much @ghiscoding for the update! 😄 The timezone dance... Here's what I'm committing today:
Thank you again for your patience and guidance, and for sharing your screenshot! It's exciting to see RTL support coming together across the ecosystem.
|
2b49c2c to
1c3a8ae
Compare


This PR adds foundational RTL (Right-to-Left) support to SlickGrid, focusing on:
Changes
hideSidegetter for dynamic CSS property selection[hideSide]applyColumnWidthsto swapleft/rightfor RTLexample1-simple-rtl.htmlwith Persian sample dataTesting
Manual testing confirmed:
Breaking Changes
None.
rtloption defaults tofalse.Cypress Test Result: https://github.com/user-attachments/files/30840967/test-results-chrome.txt
Demo: https://github.com/user-attachments/assets/903c56a2-8472-43ed-8615-e567146685a2