[Draft] Feature: Add animation sound playback selector demo#3218
[Draft] Feature: Add animation sound playback selector demo#3218qingqing-ux wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a playback mode selector to the SoundEditor component, allowing users to choose whether a selected sound plays once or follows the animation. It also includes comprehensive unit tests for this new functionality. The review feedback highlights several improvement opportunities: removing the UITooltip wrapper around the select element to prevent instant-hover overlaps and redundant information, cleaning up the unused playbackTooltip computed property and UITooltip import, and avoiding absolute positioning for the playback selector to prevent potential layout and scrolling issues.
| <UITooltip :delay="0"> | ||
| {{ $t(playbackTooltip) }} | ||
| <template #trigger> | ||
| <UISelect | ||
| v-radar="{ | ||
| name: 'Animation sound playback selector', | ||
| desc: 'Select how the selected sound plays with the animation' | ||
| }" | ||
| :value="selectedPlayback" | ||
| class="w-[118px] px-3" | ||
| @update:value="handlePlaybackUpdate" | ||
| > | ||
| <UISelectOption value="once"> | ||
| {{ $t({ en: 'One', zh: '一次' }) }} | ||
| </UISelectOption> | ||
| <UISelectOption value="follow-animation"> | ||
| {{ $t({ en: 'Follow animation', zh: '跟随动画' }) }} | ||
| </UISelectOption> | ||
| </UISelect> | ||
| </template> | ||
| </UITooltip> |
There was a problem hiding this comment.
Wrapping the UISelect component with a UITooltip that has :delay="0" can cause usability issues. The tooltip will pop up instantly on hover, which can overlap with the dropdown menu options and interfere with the user's ability to click and select options. Additionally, the tooltip content is redundant because the selected option text ('One' or 'Follow animation') is already clearly visible. Removing the tooltip simplifies the UI and improves usability.
<UISelect
v-radar="{
name: 'Animation sound playback selector',
desc: 'Select how the selected sound plays with the animation'
}"
:value="selectedPlayback"
class="w-[118px] px-3"
@update:value="handlePlaybackUpdate"
>
<UISelectOption value="once">
{{ $t({ en: 'One', zh: '一次' }) }}
</UISelectOption>
<UISelectOption value="follow-animation">
{{ $t({ en: 'Follow animation', zh: '跟随动画' }) }}
</UISelectOption>
</UISelect>
| const playbackTooltip = computed(() => | ||
| selectedPlayback.value === 'follow-animation' | ||
| ? { en: 'Follow animation', zh: '跟随动画' } | ||
| : { en: 'Play Once', zh: '播放一次' } | ||
| ) |
| UISelect, | ||
| UISelectOption, | ||
| UITooltip | ||
| } from '@/components/ui' |
| <div v-if="selected != null" class="absolute bottom-4 left-4 flex h-8 items-center gap-2"> | ||
| <span class="text-base text-grey-900"> | ||
| {{ $t({ en: 'Playback', zh: '播放' }) }} | ||
| </span> |
There was a problem hiding this comment.
Using absolute positioning (absolute bottom-4 left-4) inside a potentially scrollable container (like UIDropdownForm's body) can lead to layout bugs. If the list of sounds is long and the form scrolls, the absolutely positioned element might scroll with the content or get clipped by the container's overflow boundaries.
Consider if there is a way to avoid absolute positioning, such as limiting the scrollable area to the <ul> list itself (using overflow-y-auto on the <ul>) and keeping the playback selector as a static block element below it.
There was a problem hiding this comment.
The overall structure is clean and the tests cover the happy paths well. A few issues to address before shipping.
Critical: The Loop playback option is surfaced to users but the effect is silently discarded at save time. Animation.export() in animation.ts builds config.onStart = { play: soundName } and never writes the loop field (the existing TODO comments explain why — pending goplus/spx#1574). This means a user who selects Loop, confirms, and reloads the project will find the setting gone. Either disable / hide the Loop option with a "coming soon" tooltip until the spx issue is resolved, or resolve it before this PR lands.
Important: selectSound only resets selectedPlayback to 'once' when selected.value == null (first selection). When the user switches from Sound A → Sound B, the previous playback mode carries over silently. Intentional? If so, please add a comment explaining the design choice.
Minor: The English label for value="once" is "One", but the tooltip reads "Play Once". These two strings present inconsistent vocabulary to the user — "Once" would align with the tooltip.
| @update:value="handlePlaybackUpdate" | ||
| > | ||
| <UISelectOption value="once"> | ||
| {{ $t({ en: 'One', zh: '一次' }) }} |
There was a problem hiding this comment.
Label "One" is inconsistent with the tooltip text "Play Once" (line ~113). Consider using "Once" here so the dropdown option and the tooltip describe the same thing with the same word.
| : { en: 'Play Once', zh: '播放一次' } | ||
| ) | ||
|
|
||
| function selectSound(sound: string) { |
There was a problem hiding this comment.
Playback mode resets to 'once' only on the very first selection (selected == null). When the user switches from one sound to another, the previous playback setting silently carries over. If this is intentional, please add a comment; otherwise reset selectedPlayback on every sound switch:
| function selectSound(sound: string) { | |
| function selectSound(sound: string) { | |
| if (selected.value == null || selected.value !== sound) selectedPlayback.value = 'once' | |
| selected.value = sound | |
| } |
|
|
||
| await wrapper.findAll('.sound-item')[0].trigger('click') | ||
| await nextTick() | ||
|
|
There was a problem hiding this comment.
This assertion couples the test to UISelect's internal root class name (.ui-select). If UISelect is ever refactored, this line breaks for a reason unrelated to SoundEditor. Consider removing it or scoping it to just expect(wrapper.find('.ui-select').exists()).toBe(true) to verify the select is rendered, not its internal class structure.
| const sound1 = new Sound('Sound01', mockFile()) | ||
| const sound2 = new Sound('Sound02', mockFile()) | ||
| project.addSound(sound1) | ||
| project.addSound(sound2) |
There was a problem hiding this comment.
animationInits?.sound ?? undefined converts a potential null to undefined unnecessarily. Per project conventions, null represents absence — use ?? null here:
| project.addSound(sound2) | |
| sound: animationInits?.sound ?? null, |
[skip review]
This is a frontend-only UI demo PR and is expected to stay in draft until the related model/export flow lands.
Issue
Updates #3214
Background
Animation sound binding needs a UI demo so users can choose whether a selected sound plays once independently or follows animation playback.
Changes
Scope
Design System Impact