feat(content-sidebar): focused state for threaded-replies in activity-feed-v2#4650
feat(content-sidebar): focused state for threaded-replies in activity-feed-v2#4650mrscobbler wants to merge 3 commits into
Conversation
WalkthroughActivityFeedV2 forwards ChangesActivity feed focus state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx (1)
155-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
keyon the single root wrapper<div>is redundant.React only uses
keyto reconcile siblings in a list. Since this<div>is the sole element returned byFeedItemRow, and the list key is already applied whereFeedItemRowis rendered (ActivityFeedV2.tsxLine 363), thekey={item.id}here has no effect. Harmless, but can be dropped for clarity.Also applies to: 220-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx` at line 155, The single root wrapper div in FeedItemRow has a redundant key prop, since React only uses keys among sibling list items and the parent render in ActivityFeedV2 already provides the list key. Remove key={item.id} from the root div in FeedItemRow (and the similar duplicate occurrence in the same component) while keeping the existing className and structure unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx`:
- Line 155: The single root wrapper div in FeedItemRow has a redundant key prop,
since React only uses keys among sibling list items and the parent render in
ActivityFeedV2 already provides the list key. Remove key={item.id} from the root
div in FeedItemRow (and the similar duplicate occurrence in the same component)
while keeping the existing className and structure unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07e8e2aa-1785-4faf-afb0-08f7e8da4ce2
📒 Files selected for processing (3)
src/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.scsssrc/elements/content-sidebar/activity-feed-v2/ActivityFeedV2.tsxsrc/elements/content-sidebar/activity-feed-v2/FeedItemRow.tsx
…-feed-v2 When a comment or annotation is deep-linked via activeFeedEntryId, paint a focused chrome on the matching thread by recoloring the vendor card's existing border + background. The wrapper around ThreadedAnnotation is display: contents by default so layout is untouched for unfocused rows and the vendor's gap/padding contract is preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s focus - Use Blueprint tokens (--bp-box-blue-80, --bp-box-blue-opacity-04) and drop the now-unused variables import - Remove redundant key from the single-root threadRow wrapper divs - Add focus-state tests covering the threadRow wrapper and is-focused toggling Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
0496e01 to
1f47211
Compare
| &.is-focused [class*='threadedAnnotations'] { | ||
| border-color: var(--bp-box-blue-80); | ||
| background-color: var(--bp-box-blue-opacity-04); | ||
| } |
There was a problem hiding this comment.
I think these styles should live in the shared feature, not the UI Element. That way the styles are located in one codebase instead of spread across repos and then other consumers like notes won't need to duplicate this.
The other issue is that the threadedAnnotations wildcard is fragile since it relies on an internal classname that is not guaranteed by the component.
you could have ActivityFeed.List.ThreadedAnnotation receive an isFocused prop that forces the style on and then still set it with item.id === activeFeedEntryId
There was a problem hiding this comment.
We can't have an actual focus state on the threaded annotations component itself due to the internal components also requiring focus state (@jackiejou keep me honest here). If we added an isFocused state it could be misleading? Open to moving to this strategy though if we're ok with this treatment not actually being used for accessibility related focus state-like behavior.
There was a problem hiding this comment.
Did a little more analysis:
The threaded annotations component already owns real focus semantics (:focus-within, root outline). This styling in this PR is a passive selected highlight, not focus — isFocused would collide and could get wired to outlines/aria later.
Proposal: add isActive (or isSelected) to the shared component, docs as selection-only (border + tint, no outline), scoped :not(.annotations) so the inline drawing/region popup is untouched.
The shared component is used across two surfaces -- the sidebar + annotations in a note/document/video/image -- so we'll have to get specific with the css on when the active state is activated vs not.
@tjuanitas thoughts ?
There was a problem hiding this comment.
yeah an isSelected or isHighlighted prop in ActivityFeed.List.ThreadedAnnotation could work
something like this:
// ActivityFeedV2.tsx
filteredItems.map(item => (
<FeedItemRow
key={item.id}
isSelected={activeFeedEntryId === item.id} // calculate the condition here instead of in FeedItemRow
...
/>
// FeedItemRow.tsx
<ActivityFeed.List.ThreadedAnnotation
isSelected={isSelected} // passthrough prop
...
/>
// Handle the styles in ThreadedAnnotationsV2
There was a problem hiding this comment.
Ok -- we're taking that approach! Will close out this PR.
5 participants
Summary
activeFeedEntryIdmatches the row id.@box/threaded-annotationsor@box/activity-feed— overrides the vendor card's border + background only when the wrapper has.is-focused.display: contentsby default so unfocused layout is byte-identical to today.Test plan
<li data-activity-id>is still emitted (scroll-to-id still works) — key now lives on the wrapper divFollow-up: scroll-block tuning (
{ block: 'center' }) for deep-links depends on an@box/activity-feedAPI change tracked separately.🤖 Generated with Claude Code
Summary by CodeRabbit
activeFeedEntryIdto drive the selected-thread styling.is-focusedclass only whenactiveFeedEntryIdmatches (including theundefinedcase).