PersonMenu + redesign of the SidePanel's 'Identified People' section - #22
Merged
Conversation
Row buttons get an explicit aria-label since AvatarPill's avatar-initials span would otherwise leak into the computed accessible name.
… 'Nuevo' to its own row
Reviewer's GuideIntroduces a reusable PersonMenu surface for selecting people/roles and wires it into the SidePanel’s people section, redesigning the “Personas sugeridas” area into “Personas identificadas” with a two-row layout and an optional popover-based menu for the “Nuevo” action, plus updated stories, tests, public exports, and a version bump to 0.6.0. Sequence diagram for SidePanel Nuevo flow with optional PersonMenusequenceDiagram
actor User
participant SidePanel
participant Popover
participant PersonMenu
participant Parent
User->>SidePanel: click Nuevo button
alt newPersonOptions provided
SidePanel->>Popover: setMenuOpen(true)
Popover->>PersonMenu: render(options=newPersonOptions)
alt user chooses role option
User->>PersonMenu: click role button
PersonMenu->>SidePanel: onSelectOption(index)
SidePanel->>Parent: onSelectNewPersonOption(index)
SidePanel->>Popover: setMenuOpen(false)
else user chooses Nueva persona
User->>PersonMenu: click footer button
PersonMenu->>SidePanel: onFooterAction()
SidePanel->>Parent: onNewPerson()
SidePanel->>Popover: setMenuOpen(false)
end
else no newPersonOptions
SidePanel->>Parent: onNewPerson()
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
WithoutNewPersonMenuthe story description says "Nuevo" callsonNewPersondirectly, but the args don’t passonNewPerson, so the button ends up inert; consider wiring a no-op or example handler so the Storybook behavior matches the description. - The
ResizeObserverstub and Element prototype patches introduced inSidePanel.test.tsxare effectively global (viabeforeAll+vi.stubGlobal); if other tests rely on real behavior or different stubs, it might be safer to move this into a shared test setup or make it more scoped/resettable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `WithoutNewPersonMenu` the story description says "Nuevo" calls `onNewPerson` directly, but the args don’t pass `onNewPerson`, so the button ends up inert; consider wiring a no-op or example handler so the Storybook behavior matches the description.
- The `ResizeObserver` stub and Element prototype patches introduced in `SidePanel.test.tsx` are effectively global (via `beforeAll` + `vi.stubGlobal`); if other tests rely on real behavior or different stubs, it might be safer to move this into a shared test setup or make it more scoped/resettable.
## Individual Comments
### Comment 1
<location path="src/components/person-menu/PersonMenu.tsx" line_range="98-100" />
<code_context>
+ className,
+ "aria-label": ariaLabel = "Personas",
+}: PersonMenuProps) {
+ const footer =
+ footerSlot ??
+ (footerLabel ? (
+ <Button
+ variant="tertiary"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid rendering a clickable footer button when there is no `onFooterAction` handler.
Currently the footer button renders whenever `footerLabel` is set, even if `onFooterAction` is `undefined`, resulting in a clickable control that does nothing. Either gate rendering on `footerLabel && onFooterAction` or require `onFooterAction` whenever `footerLabel` is provided so the footer button always triggers an action.
Suggested implementation:
```typescript
const footer =
footerSlot ??
(footerLabel && onFooterAction ? (
<Button
variant="tertiary"
size="sm"
onClick={onFooterAction}
className={footerButton}
>
<PlusIcon size={16} />
{footerLabel}
</Button>
) : null);
```
You may also want to update the `PersonMenuProps` type so that `footerLabel` and `onFooterAction` are modeled as a pair (e.g., via a discriminated union or by making them both required in a specific variant), ensuring at the type level that a label cannot be provided without a handler.
</issue_to_address>
### Comment 2
<location path="src/components/side-panel/SidePanel.stories.tsx" line_range="261-266" />
<code_context>
+};
+
+/** Sin `newPersonOptions`: "Nuevo" llama a onNewPerson directo (retrocompatible). */
+export const WithoutNewPersonMenu: Story = {
+ args: {
+ people: [{ id: "s1", initials: "P1", name: "Persona 1", renamable: true }],
+ timestamp: "01:15",
+ turn: { initials: "P1", name: "Persona 1", time: "01:15", color: "violet" },
+ newPersonOptions: undefined,
+ },
+};
</code_context>
<issue_to_address>
**nitpick (bug_risk):** `WithoutNewPersonMenu` story leaves the 'Nuevo' button without an action.
The description says this variant should call `onNewPerson` directly, but the story doesn’t pass `onNewPerson` in `args`, so the "Nuevo" button is inert. Either wire a no-op `onNewPerson` to show the intended behavior, or remove the button when no handler is provided.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Gate PersonMenu's footer button on both footerLabel and onFooterAction so a future consumer that passes only footerLabel doesn't get a dead button. - Wire onNewPerson in the WithoutNewPersonMenu story so it actually demonstrates the backwards-compatible fallback it documents.
- Standardize the demo role options in PersonMenu and SidePanel stories to Juez/a, Fiscal, Defensor/a, Denunciante, Acusado/a. - Translate all Spanish code comments introduced by this feature to English. User-facing strings stay in Spanish per the library's existing i18n debt.
jansaldo
changed the title
Three of the four example descriptions used a plain hyphen while one used the middle dot; standardize on '·' so Storybook's reference examples match the separator convention consumers should follow.
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
PersonMenu, a presentational surface (card +AvatarPillrows + footer action) with no anchoring/popover logic of its own, exported from the public barrel.SidePanelsection from "Personas sugeridas" to "Personas identificadas" and splits its layout into two rows: pills on top, "Nuevo" always below.Popovertrigger that opensPersonMenuwithnewPersonOptions(e.g. judicial roles) when provided; falls back to callingonNewPersondirectly when omitted — fully backwards compatible.0.6.0and tagsv0.6.0.Unrelated minor patch
ArchiveRow.stories.tsx: standardized the exampledescriptionstrings to use·instead of a mix of-/·(three of the four examples used a hyphen). Doesn't touchArchiveRowitself — it only renders whateverdescriptionstring it's given; this just fixes the reference examples in Storybook to reflect the separator convention consumers (e.g.desktop-app) should follow.Test plan
vitest run— 72 tests passing (incl. newPersonMenu.test.tsxandSidePanel.test.tsx)tsc -p tsconfig.build.json --noEmit— cleanbiome check— clean (no non-token CSS values;strictTokens: trueenforced)vite build && panda cssgen—PersonMenupresent indist/, no arbitrary-value violationsstorybook build— smoke-tested, all new stories compile40002701:44844,40002701:44829) — not done in this non-interactive environment; recommend a manual pass instorybook devbefore merging.Spec:
docs/superpowers/specs/2026-08-06-person-menu-personas-identificadas-design.mdPlan:
docs/superpowers/plans/2026-08-06-person-menu-personas-identificadas.md🤖 Generated with Claude Code