[CLASS-13991] Auto-mirror directional PhosphorIcons in RTL - #3174
[CLASS-13991] Auto-mirror directional PhosphorIcons in RTL#3174maddy531 wants to merge 10 commits into
Conversation
🦋 Changeset detectedLatest commit: 976777e The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
|
Size Change: +1.54 kB (+1.17%) Total Size: 133 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (d0d76a0) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3174"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3174 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-qtnzumbicw.chromatic.com/ Chromatic results:
|
1d78b30 to
b85ba7e
Compare
d16c894 to
f124ade
Compare
There was a problem hiding this comment.
I added a custom icon for testing purposes/for storybook.
|
@claude review once |
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
Thank you for working on this @maddy531! This is a great idea, and will improve our rtl support!
I left some comments in the PR - The implementation overall looks good to me, though I wanted to discuss more how this will be rolled out since there are considerations for breaking changes!
| * whitelist (e.g. a custom directional SVG). Defaults to whitelist | ||
| * behavior when omitted. Does not disable mirroring for whitelisted icons. | ||
| */ | ||
| mirrorInRtl?: boolean; |
There was a problem hiding this comment.
Idea for later: It would be great to support the mirrorInRtl prop in the Icon component too (which is used for custom icons that are multi-colored)!
There was a problem hiding this comment.
Created a ticket for this idea!
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline nits, I also checked whether shouldMirrorIconInRtl fails safe when a bundler inlines a small SVG as a data: URI (some production configs do this for small assets) — it returns false in that case rather than throwing or matching incorrectly, so the icon simply isn't mirrored rather than breaking. Not a bug, just a note on what was checked.
Extended reasoning...
This is a brief informational note rather than a full review, per the ruled-out-issue exception: bugs were already found and posted as inline comments (test convention nit, changeset minor-vs-major nit), so a full approve/defer verdict isn't warranted here. I additionally verified the data-URI inlining edge case in directional-icons.ts — getPhosphorIconName explicitly returns undefined for any string starting with data:, and shouldMirrorIconInRtl treats that as false, so if a bundler ever inlines a whitelisted icon as a data URI, the icon silently renders unmirrored rather than crashing or matching the wrong icon. That's a reasonable fail-safe default given there's no way to recover a name from a data URI, so I did not flag it as a bug.
72980b9 to
28788bc
Compare
|
@claude review once |
There was a problem hiding this comment.
I reviewed this PR again and found no bugs this pass — the changeset is now a major bump and the new test file includes Arrange/Act/Assert comments, addressing the two issues flagged in my earlier review. Given this is a breaking change to icon rendering that requires coordinated releases across wonder-blocks, perseus, and frontend, a human look is still worthwhile before merging.
Extended reasoning...
No bugs were found by the bug hunting system this run, and the two issues I previously flagged (changeset version bump and missing Arrange/Act/Assert comments in the new test file) have both been addressed in the current diff. The candidate issues raised and ruled out this run (div vs View in a docs-only story, the intentional caret-swap example in link.stories.tsx, the arrow-square-down-left/right exclusion rationale, and an import-order nit) were all examined and are not real bugs, consistent with the bug hunting system's conclusion.\n\nThis PR is nonetheless a major, breaking behavioral change: PhosphorIcon now auto-mirrors any allowlisted directional icon in RTL, which double-flips any existing isRtl-based icon swap at a call site. The rollout is explicitly coordinated across three repositories (wonder-blocks, perseus, frontend) with a specific sequencing requirement to avoid a visible regression window. The core logic (bundler-specific icon name extraction and allowlist matching in directional-icons.ts) is non-trivial but has thorough test coverage for edge cases (hashed filenames, weight suffixes, data URIs, partial-segment matches).\n\nGiven the breaking-change scope, the multi-repo coordination requirement, and that a human reviewer (beaesguerra) has been actively engaged in substantive design discussion on this PR (versioning strategy, the mirrorInRtl override, and the arrow-square-up-left exclusion), this falls squarely in the category of a large-scale design decision that warrants human sign-off rather than automated approval, even though no new bugs were found.
jandrade
left a comment
jandrade
left a comment
There was a problem hiding this comment.
Looks great! Thanks for adding this useful functionality in WB 👏 🚀
| <PhosphorIcon icon={caretRightIcon} mirrorInRtl={false} /> | ||
| ``` | ||
|
|
||
| <Canvas of={PhosphorIconRtlStories.CustomIconMirrorInRtl} /> |
There was a problem hiding this comment.
suggestion: Could you please add a title and a description for how this is used with custom icons? thanks!
| * Icon names mirrored in RTL by `PhosphorIcon`. Single source of truth — | ||
| * add or remove names in a Wonder Blocks PR rather than special-casing a call | ||
| * site. `mirrorInRtl` exists for the exceptions. See the PhosphorIcon RTL docs | ||
| * for deliberate exclusions (media, launch, trends, physical text-align, etc.). |
There was a problem hiding this comment.
suggestion: I'd recommend adding a link to https://phosphoricons.com/ so folks know what icon names are expected to be included.
| Which icons mirror is decided by the allowlist in | ||
| `packages/wonder-blocks-icon/src/util/mirrored-icon-names.ts`. To add or remove | ||
| an icon, open a Wonder Blocks PR against that list rather than handling RTL | ||
| mirroring at the call site. For genuine one-offs, `mirrorInRtl` overrides the | ||
| allowlist in either direction. |
There was a problem hiding this comment.
suggestion: I'd recommend adding a link to https://phosphoricons.com/ so folks know what icon names are expected to be included.
Whitelists reading-direction glyphs and flips them under :dir(rtl). Which icons mirror is owned by Wonder Blocks — update directional-icons.ts rather than handling RTL at the call site. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the RTL whitelist as an explicit source of truth so PhosphorIcon and follow-on tooling (e.g. lint) can share the same list without editing the matching helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
…sset names for RTL icon mirroring Chromatic/Storybook production builds emit `arrow-right-[hash].svg`. Suffix-only matching missed those, so glyphs never got the mirror class even though flex layout reversed under dir=rtl. Co-authored-by: Cursor <cursoragent@cursor.com>
…ow automatically mirrors directional icons in RTL via a central allowlist, so call sites must stop swapping icons on `isRtl` (or pass `mirrorInRtl` to override per call site) to avoid double-flipping.
7e36805 to
976777e
Compare
3 participants
Summary
PhosphorIconnow auto-mirrors icons whose glyph encodes a reading direction in RTL, driven by a central allowlist.mirrorInRtloverrides the allowlist per call site in both directions:truemirrors a custom directional SVG,falseopts an allowlisted icon out.falseis for absolute directions — e.g. perseus's graph-editor arrows, where "left" means decreasing x in every locale.Issue: CLASS-13991
Follow-up: CLASS-14877 (same support in the
Iconcomponent)Breaking change
Released as major. Any consumer doing
isRTL ? caretLeft : caretRightwill double-flip in RTL after upgrading, and must pass the LTR-facing icon instead (ormirrorInRtl={false}).Rollout
wonder-blocks-icon, drop the 2scrollable-view.tsxswaps, and opt the graph editor's x-axis arrows out withmirrorInRtl={false}. Publish.@khanacademy/perseusand@khanacademy/wonder-blocks-icontogether (both are exact-pinned in the samepnpm-workspace.yamlcatalog), and remove the 11 caret/arrow swaps.Test plan
pnpm testpnpm lintpnpm typecheckExpectedToMirror/ExpectedNotToMirrorcover the allowlistin LTR and RTL panels;
CustomIconMirrorInRtlcovers the override