Repository navigation
Conversation
…x RTL overlap Three Avatar/Avatars defects from the #257 deep review (issue #265): - Avatar: the erroredSrc latch was never cleared, so switching src away and back to a once-failed URL never retried — initials forever. The error state now resets whenever src changes (render-time adjustment), making failure per-URL-attempt instead of per-mount history. - Avatars: React.Children.toArray does not descend into Fragments, so a fragment wrapper counted as ONE child — max never clamped and the group size/shape was cloned onto the fragment and dropped. Fragments are now flattened recursively, re-keyed with the fragment's key as a prefix to avoid cross-level key collisions. - _avatars.scss: overlap used physical margin-left, which pulls avatars apart instead of overlapping under dir="rtl". Now margin-inline-start (both the overlap and the .is-spaced reset). Compiled-SCSS style tests pin the logical property; a new RTLOverlap story shows it live. Drive-by: the 'ignores non-element children' test now asserts the text node is actually dropped (it previously passed with or without the filter). Badge's is-top-right/left position variants keep physical left/right deliberately — those names promise physical corners. Fixes #265 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pohc8xLkdx4gwXkW3xd7up
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Walkthrough
ChangesAvatar component behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
|
Closing: #297 merged the same three #265 fixes (Avatar src retry, Fragment flattening, RTL logical margins) at 03:57 and released as 5.4.2 — this branch was a parallel implementation and now conflicts. The two small extras here that #297 didn't include (avatar.md/avatars.md notes, a compiled-SCSS style test pinning Generated by Claude Code |
2 participants
Pull Request
Description
Fixes the three Avatar/Avatars behavior defects from issue #265 (found by the #257 deep review), plus its drive-by test fix. Supersedes #296 (same branch/commit, closed for authorship).
@allxsmith/bestax-bulma)create-bestax)@allxsmith/bestax-docs) — avatar.md / avatars.md notes1.
erroredSrclatch never cleared (Avatar.tsx). The failed-src marker was only ever set, sosrc="/a.jpg"fails → switch to/b.jpg→ switch back to/a.jpgstayed on initials forever with no retry. The error state now resets wheneversrcchanges (React render-time state adjustment — no effect, no extra commit), making the failure per-URL-attempt instead of per-mount history. Test: fail → switch → switch-back asserts the img remounts with the original URL.2. Fragment children broke
maxand prop injection (Avatars.tsx).React.Children.toArraydoes not descend into Fragments, so<Avatars max={3}><>{…mapped list…}</></Avatars>counted ONE child:maxnever clamped, no surplus bubble, and the groupsize/shapewas cloned onto the Fragment and dropped. Fragments are now flattened recursively before counting; flattened elements are re-keyed with their fragment's key as a prefix so children hoisted from different nesting levels can't collide with same-position siblings (test asserts no React duplicate-key warning for the static-avatar + keyed-mapped-list case from the issue).3. RTL overlap broken (
_avatars.scss). The stack overlap used physicalmargin-left; underdir="rtl"the negative margin pulls avatars apart instead of overlapping. Both the overlap rule and the.is-spacedreset now use logicalmargin-inline-start. Per the issue's suggestion the other #257 partials were audited:_avatar.scsshas no physical direction properties, and_badge.scss'sleft:/right:usages belong to the named position variants (is-top-rightpromises a physical corner), so they deliberately stay physical.Drive-by: the
ignores non-element childrentest now asserts the text node is actually dropped — it previously passed with or without the filter.Related Issue(s)
Closes #265
Type of Change
Checklist
main, pass with the fixes; includes a compiled-SCSS style suiteAvatars.styles.test.tsxpinning the logical property, per the Badge.styles.test.tsx precedent from fix(bulma-ui): fix standalone Badge pointer-events, pulse halo, and falsy content #295)RTLOverlapstory with adir="rtl"container)pnpm gen:catalogrun — no catalog change (no API surface change)Additional Context
Implemented directly in-session (not via the claude-fix loop) at the owner's request. RTL verified via the compiled-SCSS assertions and the new Storybook story. CI (11/11) and CodeRabbit (zero findings) already passed on this exact commit under #296.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Pohc8xLkdx4gwXkW3xd7up
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation