Add a shared direction utility to wonder-blocks-core - #3167
Draft
maddy531 wants to merge 1 commit into
Draft
Conversation
Three packages each had their own way of asking "is this layout RTL?":
an inline `closest("[dir=rtl]")` in Tabs, an inline `closest("[dir]")`
plus `getAttribute` in DatePicker, and a `useDirectionDetection` hook
that lived privately inside wonder-blocks-modal and was never exported.
Consolidate them into wonder-blocks-core:
getDirection(element?) -> "ltr" | "rtl"
isRtl(element?) -> boolean
useDirection(ref?) -> "ltr" | "rtl"
useIsRtl(ref?) -> boolean
Both a plain function and a hook are needed. Tabs resolves direction
inside a keyboard handler from `event.currentTarget`, where hooks can't
be called, while DrawerDialog and DatePicker resolve it during render.
The DOM is the only source of truth. The previous hook accepted an
explicit `direction` option intended for a `RequestInfo`-style value,
but no caller ever passed it, and a second source of truth can disagree
with the DOM while the DOM wins visually. Its `defaultDirection` option
had no caller either, so both options are dropped.
Tabs and DatePicker now also resolve document-level direction, which
their inline checks did not. That fixes RTL arrow-key navigation in Tabs
when only `document.documentElement` carries `dir="rtl"`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: e028378 The changes in this PR will be included in the next version bump. This PR includes changesets to release 33 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Size Change: +13 B (+0.01%) Total Size: 131 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (6ba50fe) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3167"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3167 |
Contributor
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-vqoeexdavi.chromatic.com/ Chromatic results:
|
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.
Summary
WB-2060. Three packages each had their own way of asking "is this layout RTL?":
wonder-blocks-tabstabs.tsx:281!!element.closest("[dir=rtl]")wonder-blocks-date-pickerdate-picker.tsx:273closest("[dir]")?.getAttribute("dir") || "ltr"wonder-blocks-modaluse-direction-detection.tsDrawerDialogonlyThis consolidates them into
wonder-blocks-core:Net −243 lines.
Two design decisions worth your attention
1. Both a function and a hook, because one caller can't use a hook. Tabs resolves direction inside a keyboard handler from
event.currentTarget— hooks are illegal there. DrawerDialog and DatePicker resolve during render. So the primitive is a plain function and the hooks are thin wrappers over it. The hooks add no behaviour today; they exist because the ticket asked for a core hook, they mark the render-time contract, and they give us one place to add reactivity (e.g. aMutationObserver) later without touching call sites.2. The DOM is the only source of truth — the
directionoption is gone. The old hook accepted an explicitdirectionintended for aRequestInfo-style value. Bea raised exactly this on the DrawerLauncher PR:She's right, and the evidence supports it: no caller ever passed that option — not in production, not in tests. Logical CSS properties resolve against the
dirattribute, so the DOM wins visually regardless; a second source could only ever disagree.defaultDirectionhad no caller either, so both options are dropped. If aRequestInfo-driven case turns up later, the right fix is to setdiron the document, which this utility then reads.Behaviour changes
Both are improvements, both are intentional, both are tested:
dir=rtl, so RTL arrow-key navigation did not work when onlydocument.documentElementcarrieddir="rtl"— which is the common real-world case, and what Wonder Blocks' own Storybook does. It works now."ltr". It now resolves document-level direction too.DrawerDialogis unchanged in behaviour — it called the hook with no ref (document-level), anduseIsRtl()with no argument does the same.One deliberately preserved subtlety: if an ancestor carries a
dirattribute whose value we don't understand (e.g.dir="auto"), we honour that ancestor and return"ltr"rather than falling through to the document. That matches the old hook, and there's a test pinning it, since it's the case most likely to regress.Also added an SSR guard (
typeof document === "undefined") that the old hook lacked.Test plan
pnpm run jest— full suite, no path filter: 236 suites pass, 0 failpnpm lint— cleanpnpm typecheck— cleanNew tests:
packages/wonder-blocks-core/src/util/__tests__/direction.test.tsand.../hooks/__tests__/use-direction.test.tsxport the 11 cases from the deleted modal test (minus thedefaultDirectionone) and add coverage forisRtland thedir="auto"case. A new Tabs test covers keyboard nav when direction comes fromdocument.documentElement.Note on
wonder-blocks-floatingFloating'srtlMirrormiddleware (on the unmergedfeature/floating-uibranch) contains a fourth copy of this sameclosest("[dir='rtl'])check. I deliberately did not touch it, since that branch isn't onmainyet. Worth adoptingisRtlthere as part of that line of work — flagging for @jandrade.Not covered
The SSR guard isn't directly tested; jsdom always provides a
document.🤖 Generated with Claude Code