Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(toast): add stubbed swipeGesture #28380

Merged
merged 8 commits into from
Oct 25, 2023
Merged

feat(toast): add stubbed swipeGesture #28380

merged 8 commits into from
Oct 25, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Oct 18, 2023

Issue number: N/A


What is the current behavior?

Toast does not support swipe gestures to dismiss.

What is the new behavior?

  • Adds a swipeGesture property to enable this behavior
  • Creates an HTML playground so I can later write E2E tests
  • Adds stubbed implementations for creating and destroying the swipe gesture on prop change, present, and dismiss
  • Adds unit tests to verify that the gesture is being created/destroyed on prop change, present, and dismiss

In cf7e897 I moved the toast config tests to separate file. For some reason this was causing issues with my new tests. This might be a Stencil bug given that I got the following error:

, getValue = (e, t) => getHostRef(e).$instanceValues$.get(t), setValue = (e, t, a, o) => {
                                     ^

TypeError: Cannot read properties of undefined (reading '$instanceValues$')
    at getValue (/Users/liamdebeasi/Ionic/ionic/core/node_modules/@stencil/core/internal/testing/index.js:508:38)
    at Toast.get [as position] (/Users/liamdebeasi/Ionic/ionic/core/node_modules/@stencil/core/internal/testing/index.js:538:13)
    at Toast.dismiss (/Users/liamdebeasi/Ionic/ionic/core/src/components/toast/toast.tsx:208:30)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:533:9)
    at processTimers (node:internal/timers:507:7)

Does this introduce a breaking change?

  • Yes
  • No

Other information

Note: This PR is missing some build artifacts. I will add those once #28379 is merged and synced into this branch.

@github-actions github-actions bot added the package: core @ionic/core package label Oct 18, 2023
@github-actions github-actions bot added the package: angular @ionic/angular package label Oct 18, 2023
@github-actions github-actions bot added the package: vue @ionic/vue package label Oct 18, 2023
@liamdebeasi liamdebeasi marked this pull request as ready for review October 19, 2023 15:49
* `bottom`: The Toast can be swiped down to dismiss.
* `middle`: The Toast can be swiped up or down to dismiss.
*/
@Prop() swipeGesture?: 'vertical';
Copy link
Contributor

Choose a reason for hiding this comment

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

~ I'm ok with this as-is, but any reason for not specifying the entire type as part of this PR? You'll have to update this again to add top, bottom and middle which will create an additional diff to review vs. being reviewed as part of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't have a reason for not adding one, but I think it makes sense to add one now for the reason you mentioned. Updated in 6c4cae6

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

What's the recommended way to test this? I wasn't able to test it locally.

@liamdebeasi
Copy link
Contributor Author

Right now this is all stubbed, so manually testing it isn't going to do much since there's no implementation associated with it. (i.e. you won't be able to do swipeGesture="vertical" and drag the toast since that's not implemented yet)

The idea behind a stub is to add just enough code to be compiled and show reviewers how this feature will be activated. In other words, this PR demonstrates the interface for the feature (props, types, methods, etc), but the implementation hasn't been added yet. In this particular case, I wanted to focus on the property setup, making it reactive, and calling the code that will create or destroy the gesture on present and dismiss. You could add some logging in present and dismiss to manually verify that this.gesture is being cleared, but I also wrote test that verify this.

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@liamdebeasi liamdebeasi merged commit 2fc3ae4 into FW-2004 Oct 25, 2023
43 checks passed
@liamdebeasi liamdebeasi deleted the 2004-prop branch October 25, 2023 14:34
@liamdebeasi liamdebeasi mentioned this pull request Nov 7, 2023
2 tasks
liamdebeasi added a commit that referenced this pull request Nov 13, 2023
Issue number: resolves #21769

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

Toast does not support swipe gestures to dismiss.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Added a `swipeGesture` property that allows users to swipe toasts
closed.
Note: This is a combination of previous PRs
#28380 and
#28402

⚠️ There is a visual glitch on iOS where dragging and having the toast
animate back to its opened position causes a flicker. This is an iOS 17
regression and is being tracked in
#28467. This bug has
been reported to and confirmed by Apple.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

⚠️ Give co-author credit to author in
#23124

---------

Co-authored-by: evgeniy-skakun <evgeniy-skakun@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants