Repository navigation
feat(er-diagram): add accessible key and relationship legend (#1401) - #1428
NewtTheWolf merged 3 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| {/* Toolbar per i controlli */} | ||
| <div className="absolute top-4 left-4 z-10 flex gap-2"> | ||
| <SchemaDiagramLegend /> |
There was a problem hiding this comment.
WARNING: Expanded legend distorts the toolbar layout
The toolbar container (absolute top-4 left-4 z-10 flex gap-2) uses the default items-stretch. When the legend expands (~150px+ tall), every sibling button (Layout toggle, Export, Show all) stretches to the same height, and the legend also shifts the existing controls to the right. The PR claims it does not interfere with existing toolbar layout. Consider items-start on the toolbar container, or render the expanded panel as an absolutely-positioned popover anchored under the trigger button.
| const containerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| // Keyboard support: Close on Escape when focus is inside | ||
| useEffect(() => { |
There was a problem hiding this comment.
WARNING: Hand-rolled global Escape listener instead of the repo's useEscapeKey
This registers a bubbling window keydown listener that closes the legend on Escape regardless of where focus is (the comment even says "when focus is inside", which is not what happens), and it bypasses the modal stack in hooks/useEscapeKey.ts. If a modal/context menu is open over the diagram, Escape will also collapse the legend, and the listener does not preventDefault. Use useEscapeKey(isOpen, () => setIsOpen(false)) (or scope the handler to containerRef via onKeyDown). Note containerRef is currently assigned but never read.
| "exportDbml": "I-export bilang DBML", | ||
| "export": "I-export" | ||
| "export": "I-export", | ||
| "legend": "Alamat", |
There was a problem hiding this comment.
WARNING: Incorrect Tagalog translation for "Legend"
"Alamat" means "address", not a map/diagram legend, so users would see a misleading label on the trigger button. Use an appropriate term such as "Gabay" or "Lehenda".
| "foreignKey": "外鍵", | ||
| "foreignKeyDesc": "引用另一個表中的主鍵或唯一鍵", | ||
| "relationship": "關聯", | ||
| "relationshipDesc": "連接子表與父表的外键引用" |
There was a problem hiding this comment.
SUGGESTION: Simplified Chinese character in Traditional Chinese locale
外键 is Simplified; the zh-TW form is 外鍵 (used correctly in foreignKey on the line above).
| "relationshipDesc": "連接子表與父表的外键引用" | |
| "relationshipDesc": "連接子表與父表的外鍵引用" |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All four earlier findings are fixed in the latest commit:
Files Reviewed (4 files)
Previous Review Summary (commit 3cabb1a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3cabb1a)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Reviewed by claude-sonnet-5.5 · Input: 244 · Output: 1.3K · Cached: 178.6K Review guidance: REVIEW.md from base branch |
…scapeKey and locales
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for explaining the actual PK/FK markers and relationships in a compact legend, and for including keyboard/Escape coverage. All three targeted tests and typecheck pass.
One small follow-up: replace the fixed schema-diagram-legend-content id with useId(). Multiple mounted diagrams currently produce the same id, so aria-controls can point at another diagram's panel.
These are follow-up nits, so I am approving this. I will leave the PR open for your manual checks before any merge.
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
I updated the branch against main and replaced the fixed legend-content ID with useId. A new two-legend regression checks that each trigger controls its own content.
The four legend tests and all 8,793 frontend tests pass, with lint and typecheck green. The earlier accessibility nit is resolved. Approved on dba7010, subject to the current CI checks.
Summary
Resolves #1401.
Adds a compact, collapsible legend component to the Schema ER Diagram explaining primary key (
Key), foreign key (Link), and relationship connector representations.Key Changes
• SchemaDiagramLegend Component (
packages/web-ui/src/components/ui/SchemaDiagramLegend.tsx):Primary Key,Foreign Key) and relationship connector edges.aria-expanded,aria-controls, visible focus outlines, and closes onEscape.bg-elevated,border-strong,text-semantic-pk,text-semantic-fk,bg-accent-primary).• Integration (
packages/web-ui/src/components/ui/SchemaDiagram.tsx):• Localization:
legend,primaryKey,primaryKeyDesc,foreignKey,foreignKeyDesc,relationship,relationshipDesc) across all 13 supported locales (de,en,es,fr,it,ja,ko,pt-BR,ru,tl,vi,zh,zh-TW).• Tests:
packages/web-ui/tests/components/ui/SchemaDiagramLegend.test.tsxverifying default collapsed state, expansion/collapse toggling, ARIA attributes, item labels and descriptions, and Escape key dismissal.Verification
•
pnpm typecheck(clean, 0 errors)•
vitest run packages/web-ui/tests/components/ui/SchemaDiagramLegend.test.tsx(3/3 pass)•
vitest run packages/web-ui/tests/utils/schemaDiagram.test.ts(27/27 pass)•
vitest run packages/web-ui/tests/pages/SchemaDiagramPage.test.tsx(3/3 pass)•
eslint&prettier --check(clean, 0 errors)•
check-theme-tokens.mjs(OK)Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.