Skip to content
Open

Sparkles #14780

Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,14 @@ export default [
files: ['kolibri/**/static/**'],
rules: CJS_RULES,
},

// SVGator-generated animation files embed minified ES2022 player code (class
// fields, ??= operator) that can't be rewritten. Overriding the parser version
// for these files only
{
files: ['**/animations/**/*.vue'],
languageOptions: {
ecmaVersion: 2022,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure there's a good reason to pin the ecmaVersion - we can set it as 'latest' in the base kolibri-format configuration.

},
},
];
Original file line number Diff line number Diff line change
Expand Up @@ -56,34 +56,40 @@
</template>
</div>

<!-- Submit button: shows only a forward-arrow icon; the aria-label
cycles through four instructional states as the sequence is built. -->
<button
type="submit"
class="submit-button"
:class="[
$computedClass({
':hover': submitEnabled
? {
backgroundColor: $themeTokens.primaryDark,
}
: {},
}),
{ pulsing: submitPulsing },
{ bouncing: arrowBouncing },
]"
data-testid="submit-button"
:aria-disabled="!submitEnabled ? 'true' : undefined"
:aria-label="submitButtonAriaLabel"
:style="submitButtonStyle"
>
<KIcon
data-testid="submit-icon"
class="submit-icon"
icon="forward"
:color="submitEnabled ? $themeTokens.textInverted : $themePalette.grey.v_300"
<div class="submit-container">
<!-- Submit button: shows only a forward-arrow icon; the aria-label
cycles through four instructional states as the sequence is built. -->
<SubmitBurstAnimation
v-if="burstVisible"
class="submit-burst"
/>
</button>
<button
type="submit"
class="submit-button"
:class="[
$computedClass({
':hover': submitEnabled
? {
backgroundColor: $themeTokens.primaryDark,
}
: {},
}),
{ pulsing: submitPulsing },
{ bouncing: arrowBouncing },
]"
data-testid="submit-button"
:aria-disabled="!submitEnabled ? 'true' : undefined"
:aria-label="submitButtonAriaLabel"
:style="submitButtonStyle"
>
<KIcon
data-testid="submit-icon"
class="submit-icon"
icon="forward"
:color="submitEnabled ? $themeTokens.textInverted : $themePalette.grey.v_300"
/>
</button>
</div>
</div>
</form>

Expand All @@ -100,6 +106,7 @@
import { picturePasswordStrings } from 'kolibri-common/strings/picturePasswords';
import useKResponsiveElement from 'kolibri-design-system/lib/composables/useKResponsiveElement';
import PicturePasswordOption from './PicturePasswordOption';
import SubmitBurstAnimation from './animations/SubmitBurstAnimation';

// Pre-compute once at module scope — PICTURE_PASSWORD_SET is static JSON so
// there is no benefit to re-deriving this array on every component mount.
Expand All @@ -111,7 +118,7 @@
export default {
name: 'PicturePasswordGrid',

components: { PicturePasswordOption },
components: { PicturePasswordOption, SubmitBurstAnimation },

setup(props, { emit }) {
const $themeTokens = themeTokens();
Expand Down Expand Up @@ -286,6 +293,7 @@
);

const arrowBouncing = ref(false);
const burstVisible = ref(false);

/**
* @public
Expand All @@ -294,26 +302,43 @@
*/
const playSuccessAnimation = () => {
const STAGGER = 150;
const DURATION = 380;
const ICON_BOUNCE_DURATION = 380;
const BURST_DURATION = 1100;
const reduce = window.matchMedia('(prefers-reduced-motion: reduce)').matches;
const stagger = reduce ? 0 : STAGGER;
const dur = reduce ? 0 : DURATION;
const iconDuration = reduce ? 0 : ICON_BOUNCE_DURATION;
const burstDuration = reduce ? 0 : BURST_DURATION;
const iconCount = sequence.value.length;

if (!iconCount) {
return Promise.resolve();
}

return new Promise(resolve => {
for (let i = 0; i < sequence.value.length; i++) {
for (let i = 0; i < iconCount; i++) {
const id = sequence.value[i];
const isLast = i === sequence.value.length - 1;
const isLast = i === iconCount - 1;
window.setTimeout(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: When prefers-reduced-motion: reduce is active, dur and stagger are zeroed (correct), but burstVisible is still set to true and the 1100ms teardown timer still runs. The SVGator player in SubmitBurstAnimation has no reduced-motion guard of its own, so the animation plays even for users who have opted into reduced motion.

Consider skipping the burst entirely when reduce is true:

if (isLast) {
  arrowBouncing.value = true;
  if (!reduce) {
    burstVisible.value = true;
    window.setTimeout(() => {
      burstVisible.value = false;
    }, 1100);
  }
}

bouncingId.value = id;
if (isLast) arrowBouncing.value = true;
if (isLast) {
arrowBouncing.value = true;
if (!reduce) {
burstVisible.value = true;
window.setTimeout(() => {
burstVisible.value = false;
}, burstDuration);
}
}
window.setTimeout(() => {
if (bouncingId.value === id) bouncingId.value = null;
if (isLast) arrowBouncing.value = false;
}, dur);
}, iconDuration);
}, i * stagger);
}

window.setTimeout(() => resolve(), (sequence.value.length - 1) * stagger + dur);
const lastStart = (iconCount - 1) * stagger;
const animationDuration = Math.max(lastStart + iconDuration, lastStart + burstDuration);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Using Math.max(lastStart + iconDuration, lastStart + burstDuration) correctly gates the Promise on whichever animation takes longer — the burst outlasts the icon bounce, so the parent caller doesn't proceed while sparkles are still on screen.

window.setTimeout(() => resolve(), animationDuration);
});
};

Expand Down Expand Up @@ -356,6 +381,7 @@
submitPulsing,
bouncingId,
arrowBouncing,
burstVisible,
handleSelect,
handleDisabledSelect,
handleSubmit,
Expand Down Expand Up @@ -463,16 +489,30 @@
border-radius: 16px;
}

.submit-button {
.submit-container {
position: relative;
display: flex;
align-items: center;
justify-content: center;
}

.submit-button {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: .submit-burst has position: absolute; top: 50%; z-index: 100 but no left coordinate. Without an explicit left: 50%; transform: translate(-50%, -50%), the horizontal position of the burst SVG depends on the flex container's current layout rather than being explicitly centered. This may work visually now but will be fragile if the container layout changes.

width: 100%;
height: 100%;
padding: 0;
border: 0;
border-radius: 8px;
transition: $core-time;
}

.submit-burst {
position: absolute;
top: 50%;
left: 50%;
z-index: 100;
transform: translate(-50%, -50%);
}

.submit-icon {
top: 0;
width: 40px;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ describe('PicturePasswordGrid', () => {
iconStyle: 'colorful',
showIconText: true,
},
stubs: ['PicturePasswordOption', 'KIcon'],
stubs: ['PicturePasswordOption', 'KIcon', 'SubmitBurstAnimation'],
});
}

Expand Down Expand Up @@ -355,12 +355,20 @@ describe('PicturePasswordGrid', () => {
await nextTick();
expect(wrapper.vm.bouncingId).toBe(3);
expect(wrapper.vm.arrowBouncing).toBe(true);
expect(wrapper.vm.burstVisible).toBe(true);

// After the final 380ms, bouncing clears and Promise resolves
// After 380ms, icon/arrow bounce is done but burst is still running
jest.advanceTimersByTime(380);
await nextTick();
expect(wrapper.vm.bouncingId).toBeNull();
expect(wrapper.vm.arrowBouncing).toBe(false);
expect(wrapper.vm.burstVisible).toBe(true);
expect(resolved).toBe(false);

// Burst duration gates completion (1100ms from last icon start)
jest.advanceTimersByTime(720);
await nextTick();
expect(wrapper.vm.burstVisible).toBe(false);
expect(resolved).toBe(true);
});

Expand Down Expand Up @@ -389,6 +397,7 @@ describe('PicturePasswordGrid', () => {
await nextTick();
expect(resolved).toBe(true);
expect(wrapper.vm.arrowBouncing).toBe(false);
expect(wrapper.vm.burstVisible).toBe(false);
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { mount } from '@vue/test-utils';
import SubmitBurstAnimation from '../animations/SubmitBurstAnimation.vue';

describe('SubmitBurstAnimation', () => {
let getElementByIdSpy;

afterEach(() => {
if (getElementByIdSpy) {
getElementByIdSpy.mockRestore();
getElementByIdSpy = null;
}
});

it('renders the burst SVG', () => {
const mockPlayer = {
ready: jest.fn(callback => callback({ play: jest.fn() })),
destruct: jest.fn(),
};
getElementByIdSpy = jest.spyOn(document, 'getElementById').mockReturnValue({
svgatorPlayer: mockPlayer,
});

const wrapper = mount(SubmitBurstAnimation);

expect(wrapper.find('svg').exists()).toBe(true);

wrapper.destroy();
});

it('calls player.destruct on unmount when available', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Explicitly testing the teardown path (not just mount) closes the loop on the prior blocking finding — this test would have caught the original player.stop() bug.

const destruct = jest.fn();
const mockPlayer = {
ready: jest.fn(callback => callback({ play: jest.fn() })),
destruct,
};
getElementByIdSpy = jest.spyOn(document, 'getElementById').mockReturnValue({
svgatorPlayer: mockPlayer,
});

const wrapper = mount(SubmitBurstAnimation);

wrapper.destroy();

expect(destruct).toHaveBeenCalled();
});
});
Loading
Loading