only emit sl-change when you stop dragging (shoelace-style#1689)

* only emit sl-change when you stop dragging * only emit sl-change when you stop dragging * prettier * add changelog entry * update changelog * update changelog * update changelog
fiqrisr · Oct 31, 2023 · 12a45eb · 12a45eb
1 parent 5e620a8
commit 12a45eb
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 20 deletions.
diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md
@@ -12,6 +12,11 @@ Components with the <sl-badge variant="warning" pill>Experimental</sl-badge> bad
 
 New versions of Shoelace are released as-needed and generally occur when a critical mass of changes have accumulated. At any time, you can see what's coming in the next release by visiting [next.shoelace.style](https://next.shoelace.style).
 
+## Next
+
+- Fixed a bug with bundled components using CDN builds not having translations on initial connect [#1696]
+- Fixed a bug where the `"sl-change"` event would always fire simultaneously with `"sl-input"` event in `<sl-color-picker>`. The `<sl-change>` event now only fires when a user stops dragging a slider or stops dragging on the color canvas. [#1689]
+
 ## 2.11.2
 
 - Fixed a bug in `<sl-carousel>` component that caused an error to be thrown when rendered with Lit [#1684]

diff --git a/src/components/color-picker/color-picker.component.ts b/src/components/color-picker/color-picker.component.ts
@@ -243,7 +243,8 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
     const container = this.shadowRoot!.querySelector<HTMLElement>('.color-picker__slider.color-picker__alpha')!;
     const handle = container.querySelector<HTMLElement>('.color-picker__slider-handle')!;
     const { width } = container.getBoundingClientRect();
-    let oldValue = this.value;
+    let initialValue = this.value;
+    let currentValue = this.value;
 
     handle.focus();
     event.preventDefault();
@@ -253,12 +254,17 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
         this.alpha = clamp((x / width) * 100, 0, 100);
         this.syncValues();
 
-        if (this.value !== oldValue) {
-          oldValue = this.value;
-          this.emit('sl-change');
+        if (this.value !== currentValue) {
+          currentValue = this.value;
           this.emit('sl-input');
         }
       },
+      onStop: () => {
+        if (this.value !== initialValue) {
+          initialValue = this.value;
+          this.emit('sl-change');
+        }
+      },
       initialEvent: event
     });
   }
@@ -267,7 +273,8 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
     const container = this.shadowRoot!.querySelector<HTMLElement>('.color-picker__slider.color-picker__hue')!;
     const handle = container.querySelector<HTMLElement>('.color-picker__slider-handle')!;
     const { width } = container.getBoundingClientRect();
-    let oldValue = this.value;
+    let initialValue = this.value;
+    let currentValue = this.value;
 
     handle.focus();
     event.preventDefault();
@@ -277,12 +284,17 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
         this.hue = clamp((x / width) * 360, 0, 360);
         this.syncValues();
 
-        if (this.value !== oldValue) {
-          oldValue = this.value;
-          this.emit('sl-change');
+        if (this.value !== currentValue) {
+          currentValue = this.value;
           this.emit('sl-input');
         }
       },
+      onStop: () => {
+        if (this.value !== initialValue) {
+          initialValue = this.value;
+          this.emit('sl-change');
+        }
+      },
       initialEvent: event
     });
   }
@@ -291,7 +303,8 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
     const grid = this.shadowRoot!.querySelector<HTMLElement>('.color-picker__grid')!;
     const handle = grid.querySelector<HTMLElement>('.color-picker__grid-handle')!;
     const { width, height } = grid.getBoundingClientRect();
-    let oldValue = this.value;
+    let initialValue = this.value;
+    let currentValue = this.value;
 
     handle.focus();
     event.preventDefault();
@@ -304,13 +317,18 @@ export default class SlColorPicker extends ShoelaceElement implements ShoelaceFo
         this.brightness = clamp(100 - (y / height) * 100, 0, 100);
         this.syncValues();
 
-        if (this.value !== oldValue) {
-          oldValue = this.value;
-          this.emit('sl-change');
+        if (this.value !== currentValue) {
+          currentValue = this.value;
           this.emit('sl-input');
         }
       },
-      onStop: () => (this.isDraggingGridHandle = false),
+      onStop: () => {
+        this.isDraggingGridHandle = false;
+        if (this.value !== initialValue) {
+          initialValue = this.value;
+          this.emit('sl-change');
+        }
+      },
       initialEvent: event
     });
   }

diff --git a/src/components/color-picker/color-picker.test.ts b/src/components/color-picker/color-picker.test.ts
@@ -1,6 +1,6 @@
 import '../../../dist/shoelace.js';
 import { aTimeout, expect, fixture, html, oneEvent } from '@open-wc/testing';
-import { clickOnElement } from '../../internal/test.js';
+import { clickOnElement, dragElement } from '../../internal/test.js';
 import { runFormControlBaseTests } from '../../internal/test/form-control-base-tests.js';
 import { sendKeys } from '@web/test-runner-commands';
 import { serialize } from '../../utilities/form.js';
@@ -31,11 +31,22 @@ describe('<sl-color-picker>', () => {
 
       await clickOnElement(trigger); // open the dropdown
       await aTimeout(200); // wait for the dropdown to open
-      await clickOnElement(grid); // click on the grid
+
+      // Simulate a drag event. "sl-change" should not fire until we stop dragging.
+      await dragElement(grid, 2, 0, {
+        afterMouseDown: () => {
+          expect(changeHandler).to.have.not.been.called;
+          expect(inputHandler).to.have.been.calledOnce;
+        },
+        afterMouseMove: () => {
+          expect(inputHandler).to.have.been.calledTwice;
+        }
+      });
+
       await el.updateComplete;
 
       expect(changeHandler).to.have.been.calledOnce;
-      expect(inputHandler).to.have.been.calledOnce;
+      expect(inputHandler).to.have.been.calledTwice;
     });
 
     it('should emit sl-change and sl-input when the hue slider is moved', async () => {
@@ -50,10 +61,22 @@ describe('<sl-color-picker>', () => {
 
       await clickOnElement(trigger); // open the dropdown
       await aTimeout(200); // wait for the dropdown to open
-      await clickOnElement(slider); // click on the hue slider
+      // Simulate a drag event. "sl-change" should not fire until we stop dragging.
+      await dragElement(slider, 20, 0, {
+        afterMouseDown: () => {
+          expect(changeHandler).to.have.not.been.called;
+          expect(inputHandler).to.have.been.calledOnce;
+        },
+        afterMouseMove: () => {
+          // It's not twice because you can't change the hue of white!
+          expect(inputHandler).to.have.been.calledOnce;
+        }
+      });
+
       await el.updateComplete;
 
       expect(changeHandler).to.have.been.calledOnce;
+      // It's not twice because you can't change the hue of white!
       expect(inputHandler).to.have.been.calledOnce;
     });
 
@@ -69,11 +92,22 @@ describe('<sl-color-picker>', () => {
 
       await clickOnElement(trigger); // open the dropdown
       await aTimeout(200); // wait for the dropdown to open
-      await clickOnElement(slider); // click on the opacity slider
+
+      // Simulate a drag event. "sl-change" should not fire until we stop dragging.
+      await dragElement(slider, 2, 0, {
+        afterMouseDown: () => {
+          expect(changeHandler).to.have.not.been.called;
+          expect(inputHandler).to.have.been.calledOnce;
+        },
+        afterMouseMove: () => {
+          expect(inputHandler).to.have.been.calledTwice;
+        }
+      });
+
       await el.updateComplete;
 
       expect(changeHandler).to.have.been.calledOnce;
-      expect(inputHandler).to.have.been.calledOnce;
+      expect(inputHandler).to.have.been.calledTwice;
     });
 
     it('should emit sl-change and sl-input when toggling the format', async () => {

diff --git a/src/internal/test.ts b/src/internal/test.ts
@@ -73,11 +73,21 @@ export async function dragElement(
   /** The horizontal distance to drag in pixels */
   deltaX = 0,
   /** The vertical distance to drag in pixels */
-  deltaY = 0
+  deltaY = 0,
+  callbacks: {
+    afterMouseDown?: () => void | Promise<void>;
+    afterMouseMove?: () => void | Promise<void>;
+  } = {}
 ): Promise<void> {
   await moveMouseOnElement(el);
   await sendMouse({ type: 'down' });
+
+  await callbacks.afterMouseDown?.();
+
   const { clickX, clickY } = determineMousePosition(el, 'center', deltaX, deltaY);
   await sendMouse({ type: 'move', position: [clickX, clickY] });
+
+  await callbacks.afterMouseMove?.();
+
   await sendMouse({ type: 'up' });
 }