feat(modal): expose dismiss intent on card modal ionDragEnd - #31002
feat(modal): expose dismiss intent on card modal ionDragEnd#31002aeharding wants to merge 3 commits into
Conversation
|
@aeharding is attempting to deploy a commit to the Ionic Team on Vercel. A member of the Team first needs to authorize it. |
e9f92a4 to
c200d78
Compare
| deltaY: detail.deltaY, | ||
| velocityY: detail.velocityY, | ||
| progress: calculateProgress(el, detail.deltaY), | ||
| snapBreakpoint: shouldComplete ? 0 : 1, |
There was a problem hiding this comment.
snapBreakpoint is specific to sheet modals, and card modals have no breakpoint concept, so exposing a "breakpoint" here is a bit misleading. Let's call it isDismissing and return a plain boolean instead of 0/1.
| snapBreakpoint: shouldComplete ? 0 : 1, | |
| isDismissing: shouldComplete, |
This also follows the convention we use for other booleans on public event details, such as isScrolling on ion-content's ScrollDetail and isAnimating on the menu interface.
One more change to go with it: please add the field to ModalDragEventDetail in modal-interface.ts so it's part of the public type:
/**
* Whether the card modal will dismiss when the drag gesture ends.
* `true` if the gesture passed the dismiss threshold, `false` if the
* modal will remain open.
*/
isDismissing?: boolean;There was a problem hiding this comment.
Adding this variable changes the ionDragEnd detail length, so the existing assertion in modal-card.e2e.ts will fail. Please update the expected length there.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
There was a problem hiding this comment.
Awesome work! I did catch an issue that I believe is blocking and some others that aren't necessarily but might be nice to fix now
| deltaY: detail.deltaY, | ||
| velocityY: detail.velocityY, | ||
| progress: calculateProgress(el, detail.deltaY), | ||
| isDismissing: shouldComplete, |
There was a problem hiding this comment.
I think there's a problem here with canDismiss. shouldComplete gets forced to false whenever canDismiss is a function or false, so this reports isDismissing: false on a modal that's about to dismiss. I built the branch locally to check: card modal with canDismiss: () => Promise.resolve(true), swipe the header down 450px, and ionDragEnd fires with {"deltaY":450,"velocityY":3.94,"progress":0.322,"isDismissing":false} immediately before ionModalDidDismiss.
I'd want this sorted before approving, because "confirm before dismiss" is the most common reason to set canDismiss in the first place, and there's no later drag event to correct the value.
|
|
||
| expect(ionDragEnd.length).toBe(1); | ||
| expect(Object.keys(dragEndEvent.detail).length).toBe(4); | ||
| expect(Object.keys(dragEndEvent.detail).sort()).toEqual([ |
There was a problem hiding this comment.
This only checks the key set, so it passes whether the implementation emits true, false, null, or a hardcoded constant. The value is the entire feature, and the canDismiss problem I flagged over in swipe-to-close.ts passes like this.
The fixture already handles both outcomes, and the suite already proves which distances land either side of the threshold: swipeToCloseModal(selector) defaults to 300px and asserts dismissal, and the test further up drags 20px with the modal staying open. So something like:
test('should report isDismissing true when the gesture dismisses the modal', async ({ page }) => {
await dragElementBy(page.locator('.modal-card ion-header'), page, 0, 300);
const ev = await ionDragEnd.next();
expect(ev.detail.isDismissing).toBe(true);
});
test('should report isDismissing false when the modal settles back open', async ({ page }) => {
await dragElementBy(page.locator('.modal-card ion-header'), page, 0, 20);
const ev = await ionDragEnd.next();
expect(ev.detail.isDismissing).toBe(false);
await expect(page.locator('ion-modal')).toBeVisible();
});I ran both against a local build with this patch applied and they pass. Worth pairing each with the visibility check so the test proves the field matches reality instead of matching itself. A third one for the canDismiss path would be good too once that's settled, #card-can-dismiss-promise-true in test/can-dismiss/index.html is already there for it.
Swapping .length).toBe(4) for Object.keys(...).sort() is a nice improvement by the way, a count wouldn't have caught a rename.
| * `true` if the gesture passed the dismiss threshold, `false` if the | ||
| * modal will remain open. | ||
| */ | ||
| isDismissing?: boolean; |
There was a problem hiding this comment.
Sheet modals never set this, so a consumer writing the obvious if (ev.detail.isDismissing) gets undefined on a sheet that is dismissing. I dragged one down to breakpoint 0 locally and got {"deltaY":180,"velocityY":10.76,"progress":0,"snapBreakpoint":0} with typeof isDismissing === 'undefined'. That's falsy, so the check quietly does nothing.
Both gestures publish through the same ionDragEnd, and sheet.ts already has what it needs in scope where it builds the detail:
isDismissing: snapBreakpoint === 0 && !canDismissBlocksGesture,Same two values moveSheetToBreakpoint derives dismissal from. The early-return onDragEnd a bit further down would need an explicit false, since it doesn't move the sheet at all.
Card ionDragMove is the other half of this, it gets nothing while sheet emits snapBreakpoint on both move and end, where the move-time meaning is exactly "what happens if you let go now". I think both are worth deciding now rather than after release, because the asymmetry is permanent once it ships. Up to you though, the issue only asked about card modals so I won't block on it.
| @@ -72,4 +72,10 @@ export interface ModalDragEventDetail { | |||
| * the gesture. | |||
| */ | |||
| snapBreakpoint?: number; | |||
There was a problem hiding this comment.
The ModalDragEventDetail block in ionic-docs at docs/api/modal.md also needs to be updated, nothing over there picks this up automatically, so as it stands this ships undocumented.
Issue number: resolves #31001
What is the current behavior?
The
ionDragEndevent on card modals emitscurrentY,deltaY,velocityYandprogress. None of these indicate whether the modal is about to dismiss, so consumers cannot react to the outcome of the gesture when the user lifts their finger.What is the new behavior?
ionDragEndnow includesisDismissingon card modals. It istruewhen the gesture passed the dismiss threshold and the modal will animate away, andfalsewhen the modal will settle back open.Sheet modals are unchanged and continue to report
snapBreakpoint.Does this introduce a breaking change?
Other information
Testing with Voyager and works great! aeharding/voyager#2227
Preview