feat(slider, range-slider): add pin feature (VIV-1611) #1736

RichardHelm · 2024-06-03T09:18:51Z

No description provided.

codecov · 2024-06-03T09:27:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d61b119) to head (a0ebb90).
Report is 1034 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #1736     +/-   ##
===========================================
  Coverage   100.00%   100.00%             
===========================================
  Files          123       342    +219     
  Lines         1562      6195   +4633     
  Branches       108       772    +664     
===========================================
+ Hits          1562      6195   +4633

Flag	Coverage Δ
unittests	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

YonatanKra

Nice!
Thanks for this split and work well done.
Because now I understand that popup is our "facade" for floatingUI it makes much more sense.
3 comments and we're done here :)

YonatanKra · 2024-06-04T03:48:20Z

libs/components/src/lib/popup/popup.spec.ts

+		it('should not continuously update position with requestAnimationFrame when false', async () => {
+			element.anchor = anchor;
+			await elementUpdated(element);
+			jest.spyOn(window, 'requestAnimationFrame');
+
+			element.open = true;
+
+			expect(window.requestAnimationFrame).toHaveBeenCalledTimes(
+				RAF_CALLS_FROM_SETTING_ATTRIBUTE
+			);
+		});


It is unclear to me what we are testing.
In the first test we call element.open = true; and it uses rAF once.
In the 2nd test, we call element.open = true;and it uses rAF twice.
Question is - why should it change only twice?
Looking at the floatingUI code it seems like what it does is keep updating the position as long as it keeps changing. Which seems to be the way things are going. So IMO the tests should be:

Mock rAF implementation to change the position on every call 3 or 4 times and then stabilize it

Show that rAF is called once when the prop is false and 3 or 4 times when the prop is true

Show that rAF is called once when the position doesn't change even when the prop is true
WDYT?

With animationFrame it should continuously update the position with rAF.
After some thought, I think this test is kind of bad either way. Popup shouldn't really test how many times rAF is called or whether floatingUI works correctly.
All I really care about is that it passes the argument to floatingUI, which has it's own tests.
So maybe we should change it back to the original?

I do not believe so.
We do care about the amount of times rAF is called.
Floating UI is an implementation detail. You can implement this on your own or switch library - the tests should still pass :)

Well, maybe, but unit tests shouldn't test dependencies for correctness like that.
I really don't care how many times rAF gets called, only that the flag gets passed.
If we replace floatingUI with something, this code needs updating which I'm fine with

Also, all the other tests in this file do not test floatingUI either. Either all them should or none

We are not testing dependencies for correctness. The fact you are using Floating UI is irrelevant. It is an implementation detail. We are testing the outcome of this - so if we removed floating UI or the corresponding code that implements this API, the test should fail.

I've updated the test now to test that it continuously calls rAF. However the test is still bad and doesn't really test anything at all

If I change the test further to test that it actually updates position I end up testing floatingUI through popup...

Testing the outcome is not always what you want to do. E.g. when you have some code that sends an email, you don't want to test that it sends an actual email. Just that it calls some other code that is responsible for sending emails.

I'd argue that it's the same here. It's not popups job to calculate positions and keep them up to date, so we just verify that calls the code that is responsible for that

YonatanKra · 2024-06-04T04:05:04Z

libs/components/src/lib/popup/popup.ts

@@ -61,7 +93,7 @@ export class Popup extends FoundationElement {
 	open = false;
 	openChanged(_: boolean, newValue: boolean): void {
 		newValue ? this.$emit('vwc-popup:open') : this.$emit('vwc-popup:close');
-		this.#updateAutoUpdate();
+		DOM.queueUpdate(() => this.#updateAutoUpdate());


Now I see that the reason we have 1 rAF in the case animationFrame is false.
I gather a few things from this:

If we delete the DOM.queueupdate and leave just the autoUpdate all tests are passing. It is either irrelevant or it should be tested in the open tests. should update position in the next animation frame or something like that.

This explains why when we set animationFrame to false, we get one rAF call. So I'd might change the animationFrame property's name to something more meaningful like softPositionAnimation or smoothPositionAnimation or gradualPositionAnimation. WDYT?

No, this change has nothing to do with animationFrame or its tests
If you delete it, the ui tests will fail. The reason is that it is wrong to call autoUpdate before open has changed in the the DOM, because floatingUI requires the floating element to be measurable. Which it isn't if open is false (display: none)

Then why not add a small unit test for it?
It should autoUpdate after open changed (e.g. after elementUpdated)

Sure, I added some tests for open

YonatanKra · 2024-06-04T04:09:02Z

libs/components/src/lib/popup/popup.ts

+	 * @internal
+	 */
+	animationFrameChanged() {
+		this.#updateAutoUpdate();


updateAutoUpdate is not a descriptive name ;)
Wouldn't updatePopupParmeters be more descriptive?
I'm open for other names as well :)

Mhm, I don't think the name is that bad. Anyway, I would say that is out of scope of this PR

Well... it made me read the function in order to just understand what it does, which makes it a bad name (always take the weak link as your test case - and I'm the weak link here ;) ).

Sure, maybe there is a better name but this review is already getting very big so I want to focus only the things actually in the PR

This review has only 4 comments ;)

rachelbt

I haven't assigned myself and saw that @YonatanKra reviewed. Adding only one comment

libs/components/src/lib/popup/README.md

libs/components/src/lib/popup/popup.spec.ts

Co-authored-by: Yonatan Kra <yonatan.kra@vonage.com>

* feat(slider): add pin feature (VIV-1611) * Update libs/components/src/lib/slider/slider.spec.ts * Rename internals * Add test for thumb mouse down * Move dragging into thumb * Refactor visible popup tests * Add test for placement strategy * Add test clicking on track while disabled * Clarify code * Refactor tests * Split tests * Add pin feature to range slider

feat(popup): support placement strategy and animation frame options (V…

5e5e7c4

…IV-1611)

RichardHelm force-pushed the VIV-1611-add-popup-options branch from 44e3d0c to 5e5e7c4 Compare June 3, 2024 09:20

deno-deploy bot deployed to Preview June 3, 2024 09:25 View deployment

deno-deploy bot deployed to Preview June 3, 2024 16:39 View deployment

Fix flaky test

9a89dc9

RichardHelm force-pushed the VIV-1611-add-popup-options branch from d229e53 to 9a89dc9 Compare June 3, 2024 18:27

deno-deploy bot deployed to Preview June 3, 2024 18:31 View deployment

YonatanKra requested changes Jun 4, 2024

View reviewed changes

rachelbt reviewed Jun 4, 2024

View reviewed changes

libs/components/src/lib/popup/README.md Show resolved Hide resolved

Add tests for open

2240f5c

deno-deploy bot deployed to Preview June 4, 2024 15:33 View deployment

Improve animationFrame test

5e3cf0a

deno-deploy bot deployed to Preview June 5, 2024 10:08 View deployment

Fix test

96035fd

deno-deploy bot deployed to Preview June 5, 2024 10:47 View deployment

Update tests for animationFrame

0dab812

deno-deploy bot deployed to Preview June 5, 2024 15:21 View deployment

YonatanKra self-assigned this Jun 6, 2024

Refactor animationFrame tests

a68315f

YonatanKra requested changes Jun 17, 2024

View reviewed changes

libs/components/src/lib/popup/popup.spec.ts Outdated Show resolved Hide resolved

libs/components/src/lib/popup/popup.spec.ts Show resolved Hide resolved

RichardHelm and others added 2 commits June 18, 2024 09:57

Update libs/components/src/lib/popup/popup.spec.ts

6cbd73e

Co-authored-by: Yonatan Kra <yonatan.kra@vonage.com>

Merge branch 'main' into VIV-1611-add-popup-options

1486267

deno-deploy bot deployed to Preview June 18, 2024 09:06 View deployment

Fix linting issues

66d6755

deno-deploy bot deployed to Preview June 18, 2024 16:26 View deployment

Fix test

7ffeb88

deno-deploy bot deployed to Preview June 18, 2024 17:07 View deployment

YonatanKra previously approved these changes Jun 19, 2024

View reviewed changes

Merge branch 'main' into VIV-1611-add-popup-options

cdc2191

deno-deploy bot deployed to Preview June 19, 2024 08:09 View deployment

Merge branch 'main' into VIV-1611-add-popup-options

ffef055

deno-deploy bot deployed to Preview June 25, 2024 08:09 View deployment

RichardHelm dismissed YonatanKra’s stale review via 44c6097 July 23, 2024 17:02

Merge branch 'main' into VIV-1611-add-popup-options

8b184a5

RichardHelm changed the title ~~feat(popup): support placement strategy and animation frame options (VIV-1611)~~ feat(slider, range-slider): add pin feature (VIV-1611) Jul 23, 2024

deno-deploy bot deployed to Preview July 23, 2024 17:14 View deployment

Update snapshots

b23f79c

deno-deploy bot deployed to Preview July 24, 2024 10:18 View deployment

Merge branch 'main' into VIV-1611-add-popup-options

a0ebb90

RichardHelm enabled auto-merge (squash) July 24, 2024 10:54

deno-deploy bot deployed to Preview July 24, 2024 10:58 View deployment

TaylorJ76 approved these changes Jul 24, 2024

View reviewed changes

RichardHelm merged commit 74944b4 into main Jul 24, 2024
15 checks passed

RichardHelm deleted the VIV-1611-add-popup-options branch July 24, 2024 11:57

github-actions bot mentioned this pull request Jul 24, 2024

chore: release main #1859

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(slider, range-slider): add pin feature (VIV-1611) #1736

feat(slider, range-slider): add pin feature (VIV-1611) #1736

RichardHelm commented Jun 3, 2024

codecov bot commented Jun 3, 2024 •

edited

Loading

YonatanKra left a comment

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 5, 2024

RichardHelm Jun 5, 2024

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 4, 2024

RichardHelm Jun 4, 2024

YonatanKra Jun 5, 2024

rachelbt left a comment

feat(slider, range-slider): add pin feature (VIV-1611) #1736

feat(slider, range-slider): add pin feature (VIV-1611) #1736

Conversation

RichardHelm commented Jun 3, 2024

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

YonatanKra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachelbt left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2024 •

edited

Loading