Repository navigation
feat(workspace): name tonight's first vamp plan on the map #1024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
seonghobae
wants to merge
35
commits into
develop
Choose a base branch
from
feat/workspace-first-vamp-plan
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 27 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
e124451
feat(workspace): name tonight's first vamp plan on the map
seonghobae c701b82
test: require real-audio vamp plan emission
seonghobae e665ae0
feat: type activity-derived vamp plan
seonghobae a6fbd9c
feat: derive vamp plan from real stem entrances
seonghobae dbf692a
docs: record real-audio vamp guidance boundary
seonghobae d349735
style(analysis): normalize vamp role model formatting
seonghobae 6ef3dd6
style(analysis): apply locked Ruff formatting to vamp plan
seonghobae 2dfb27a
fix(changelog): preserve released 0.1.1 history
seonghobae cb9c6ff
test(workspace): require localized generated vamp guidance
seonghobae 94c091c
fix(workspace): localize generated vamp guidance
seonghobae bc23aef
fix(i18n): add generated vamp guidance copy
seonghobae 7268ae7
fix(i18n): localize generated vamp guidance
seonghobae 3e0c441
test(roles): cover shared-stem vamp entrances
seonghobae bdc7791
fix(roles): preserve coarse shared-stem vamp entrances
seonghobae 0f20bc6
test(workspace): keep bounded vamp templates localizable
seonghobae 335c5db
fix(workspace): preserve bounded vamp template localization
seonghobae 49be3fe
test(workspace): cover ambiguous local vamp renderers
seonghobae 198f4fa
Merge branch 'develop' into feat/workspace-first-vamp-plan
seonghobae 95d59b4
Merge remote-tracking branch 'origin/develop' into HEAD
seonghobae addb963
style: apply pinned ruff format to roles extractor
seonghobae c03001e
test(roles): cover unknown vamp entrance fail-closed path
seonghobae 59b6cf2
docs(a11y): correct disabled-control guidance
seonghobae 8052a40
test(workspace): derive vamp labels from shared contract
seonghobae d2ee02d
test(i18n): iterate shared section labels
seonghobae cdb8ffb
test(workspace): require production vamp navigation anchor
seonghobae bb005a6
test(workspace): align vamp fixtures with production anchor
seonghobae 12a6745
fix(workspace): use production vamp navigation anchor
seonghobae fc152dc
test(workspace): reproduce missing vamp navigation anchor
seonghobae a15be5b
fix(workspace): restore shared song-structure navigation anchor
seonghobae 144ffe9
test(workspace): align reduced-motion vamp fixture with renderer id
seonghobae b8ab7b1
test(workspace): align Korean vamp fixture with renderer id
seonghobae 0cb9a54
test(workspace): align scoped vamp fixtures with renderer id
seonghobae 2ebf271
fix(workspace): enforce vamp plan provenance
seonghobae 0cedf85
fix(vamp): align persisted role contracts
seonghobae e1f4a74
chore(stack): adopt protected develop into vamp plan lane
seonghobae File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,43 @@ | ||
| ## 2024-05-19 - Replace HTML disabled with aria-disabled="true" for Accessible Tooltips | ||
| **Learning:** Native HTML `disabled` attributes completely hide elements from screen readers and block all pointer/hover events, preventing tooltips from functioning for disabled elements. | ||
| **Action:** Replace `disabled` with `aria-disabled="true"`, enforce block click handlers via `e.preventDefault()`, and add a title tooltip directly to the element to maintain full tooltip accessibility and keyboard focus support for visually impaired and mouse users. | ||
| ## 2024-05-19 - Native disabled and aria-disabled serve different accessibility needs | ||
| **Learning:** Native HTML `disabled` controls are not focusable and leave the tab order, and user activation such as clicks is suppressed. They are not correctly described as completely hidden from assistive technology, and pointer/hover behavior should not be generalized across browsers and event types. `aria-disabled="true"` keeps disabled semantics without changing focusability or suppressing behavior automatically. | ||
| **Action:** Prefer native `disabled` for native form controls when the action should be unavailable. Use `aria-disabled="true"` only when a disabled action intentionally needs to remain discoverable in keyboard navigation, and then suppress activation in application code and provide a real accessible explanation; do not rely on `title` alone for screen-reader guidance. | ||
|
|
||
| ## 2024-05-18 - Added focus visible styles for keyboard navigation | ||
| **Learning:** Interactive inline buttons (like the chord editor) and scrollable regions with `tabIndex={0}` do not automatically get focus visible styles, meaning keyboard users tabbing through won't know they are focused on them. Unlike central `<Button />` components which bake focus states in, these custom inline interactive elements need explicit focus styling. | ||
| **Action:** Always add explicit focus visible styles (e.g., `focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-cyan-300`) to custom interactive elements and scrollable regions with `tabIndex={0}` for proper keyboard accessibility. | ||
|
|
||
| ## 2024-05-24 - Visual tooltips for disabled icon-only buttons | ||
| **Learning:** Icon-only buttons with `aria-label` are accessible to screen readers, but sighted users relying on mouse hover don't get context if the `title` attribute is missing, especially when the button is disabled and its purpose is unclear (e.g. "coming soon"). | ||
| **Action:** Always add a `title` attribute mirroring the `aria-label` (or providing a specific disabled reason) to icon-only buttons so sighted users also receive explanatory tooltips on hover. | ||
|
|
||
| ## 2026-06-13 - Added screen reader text for tooltip divs | ||
| **Learning:** When using `title` attributes on non-interactive elements like icon-only `div`s for tooltips, screen readers might not announce them properly because they aren't focusable. The visual tooltip is not enough for accessibility. | ||
| **Action:** Always add a visually hidden `<span className="sr-only">[Tooltip Text]</span>` inside non-interactive elements that rely on a `title` attribute so that screen readers have text content to announce. | ||
|
|
||
| ## 2026-06-18 - Added keyboard accessibility to scrollable regions | ||
| **Learning:** Horizontally scrollable regions (like the `SectionRoadmap` component) are not accessible to keyboard-only users unless they can receive focus. Keyboard users must be able to focus the container to scroll its content using arrow keys. | ||
| **Action:** For proper keyboard accessibility in custom scrollable regions, always include `tabIndex={0}`, an appropriate `aria-label`, `role="region"`, and explicit focus visible styling (e.g., `focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-cyan-300`). | ||
|
|
||
| ## 2026-06-19 - Internationalization | ||
| **Learning:** The desktop app uses i18n via json files located in `apps/desktop/src/locales/` | ||
| **Action:** When adding new text strings, make sure to add it to all locale files. | ||
|
|
||
| ## 2026-06-25 - Native tooltips on disabled elements | ||
| **Learning:** Standard HTML `title` attributes used as tooltips do not render on elements that use Tailwind's `pointer-events-none` class, which is often applied to `disabled:` variants in Base UI and styled components. | ||
| **Action:** Do not rely on native `title` attributes for explaining disabled states on buttons with `pointer-events-none`. Instead, either use a custom tooltip component or ensure focus/interactive styles are preserved if an explanation is strictly required. | ||
|
|
||
| ## 2024-06-29 - 비활성화된 네이티브 버튼의 설명 제공 | ||
| **Learning:** 네이티브 `<button disabled>`는 포커스를 받을 수 없어 키보드 탭 순서에서 제외되고 사용자 활성화가 차단됩니다. 보조기술에는 disabled 상태로 전달될 수 있으며, `title` 툴팁이나 포인터/호버 동작은 브라우저와 이벤트 종류에 따라 달라질 수 있으므로 모든 포인터 이벤트가 차단된다고 일반화하지 않습니다. | ||
| **Action:** 비활성 액션의 이유가 모든 사용자에게 필요하면 `title`만 의존하지 말고 화면에 보이는 설명 또는 `sr-only` 등 실제 접근 가능한 텍스트를 제공하세요. 툴팁을 위해 네이티브 버튼을 wrapper로 감싸야 하더라도 wrapper에 `role="button"`을 부여해 중첩 버튼 의미를 만들지 마세요. disabled 액션을 의도적으로 탭 순서에 남겨야 하는 경우에만 `aria-disabled="true"` 패턴을 사용하고, 애플리케이션 코드에서 활성화를 직접 차단하세요. | ||
|
|
||
| ## 2024-07-01 - Testing components with focusable disabled button wrappers | ||
| **Learning:** When native disabled buttons are wrapped in a focusable `span` to provide accessible tooltips, tests that previously found and clicked the `button` (by temporarily removing the `disabled` attribute) may fail or become overly complex. It is cleaner and more accurate to query the wrapper element (e.g. via its `title`) and fire events on it, reflecting the actual accessible DOM structure. | ||
| **Action:** When testing UI components that wrap disabled buttons in a focusable span for accessibility (e.g., using a tooltip/title), use `screen.getByTitle(...)` to query the wrapper element for interactions like `fireEvent.click` rather than `screen.getByRole('button')`. | ||
|
|
||
| ## 2024-05-24 - Avoid nesting native buttons with ARIA role button on wrappers | ||
| **Learning:** Adding `role="button"` to a `span` or `div` wrapper that contains a native `<button>` element inside violates ARIA specifications. Interactive roles (like `button`) must not contain other interactive elements (even if the inner element is disabled or has `aria-hidden`), as this causes invalid/redundant accessibility trees and screen reader confusion. | ||
| **Action:** Always verify wrappers used to implement tooltips for disabled buttons are standard elements (e.g., `<span tabIndex={0} title="...">`) but *do not* assign `role="button"` to the wrapper itself. | ||
|
|
||
| ## 2026-07-02 - Inline clear buttons preserve focus | ||
| **Learning:** Inline clear buttons often unmount immediately after clearing state, which can drop keyboard focus to the document body. | ||
| **Action:** Move focus back to the owning input before clearing state, and cover the behavior with a DOM focus test. |
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
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
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
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
21 changes: 21 additions & 0 deletions
21
apps/desktop/src/features/workspace/FirstVampPlanCallout.identity.test.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import { render, screen } from "@testing-library/react"; | ||
| import { createDemoRehearsalSong } from "@bandscope/shared-types"; | ||
| import { expect, it } from "vitest"; | ||
| import { FirstVampPlanCallout } from "./FirstVampPlanCallout"; | ||
|
|
||
| it("gives co-mounted vamp-plan callouts distinct DOM identities", () => { | ||
| render( | ||
| <> | ||
| <FirstVampPlanCallout song={createDemoRehearsalSong()} /> | ||
| <FirstVampPlanCallout song={createDemoRehearsalSong()} /> | ||
| </> | ||
| ); | ||
|
|
||
| const callouts = screen.getAllByRole("complementary", { | ||
| name: "Tonight's first vamp plan" | ||
| }); | ||
| const ids = callouts.map((callout) => callout.id); | ||
|
|
||
| expect(ids.every((id) => id.length > 0)).toBe(true); | ||
| expect(new Set(ids).size).toBe(callouts.length); | ||
| }); |
25 changes: 25 additions & 0 deletions
25
apps/desktop/src/features/workspace/FirstVampPlanCallout.memoization.test.tsx
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| import { render } from "@testing-library/react"; | ||
| import { createDemoRehearsalSong } from "@bandscope/shared-types"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
| import { FirstVampPlanCallout } from "./FirstVampPlanCallout"; | ||
|
|
||
| describe("FirstVampPlanCallout resolver reuse", () => { | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it("does not rescan role metadata when a parent rerenders the same song object", () => { | ||
| const song = createDemoRehearsalSong(); | ||
| const role = song.sections[0]!.roles.find((candidate) => candidate.id === "lead-vocal")!; | ||
| const descriptorSpy = vi.spyOn(Object, "getOwnPropertyDescriptor"); | ||
|
|
||
| const { rerender } = render(<FirstVampPlanCallout song={song} />); | ||
| const firstScanCount = descriptorSpy.mock.calls.filter(([target]) => target === role).length; | ||
| expect(firstScanCount).toBeGreaterThan(0); | ||
|
|
||
| rerender(<FirstVampPlanCallout song={song} />); | ||
| const secondScanCount = descriptorSpy.mock.calls.filter(([target]) => target === role).length; | ||
|
|
||
| expect(secondScanCount).toBe(firstScanCount); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.