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

update delta to allow different thresholds for each side #260

Merged
merged 3 commits into from
Aug 13, 2021

Conversation

macabeus
Copy link
Contributor

@macabeus macabeus commented Jul 7, 2021

Closes #258

Now it's possible to set a different delta for each side:

<SwipeableUsingHook
  delta={{
    right: 10,
    left: 20,
    up: 30,
    down: 40,
  }}
/>

It also exports a new type: ConfigurationOptionDelta. As requested by @hartzis, delta's type is tied with dir's type:

export type ConfigurationOptionDelta =
  | number
  | { [key in Uncapitalize<SwipeDirections>]: number };

@hartzis hartzis self-assigned this Jul 27, 2021
@hartzis
Copy link
Collaborator

hartzis commented Jul 27, 2021

@macabeus Thank you for setting up the PR for this. My tentative goal is try to look at this this week or next.

It does appear to be failing the build so i'm curious what that is related to, possibly our version of typescript and/or microbundle. We may need to Upgrade to microbundle@0.13.0, which uses typescript@4 like #241 does 🤔

@macabeus macabeus force-pushed the feat/custom-thresholds branch from d2f99c8 to 23d456e Compare July 27, 2021 17:10
@macabeus macabeus force-pushed the feat/custom-thresholds branch from 23d456e to 27430f7 Compare July 27, 2021 17:10
@macabeus
Copy link
Contributor Author

macabeus commented Jul 27, 2021

@hartzis Okay. I did a cherry-pick with @stefvhuynh's commit, updating microbundle to 0.13.3.
With this bump, I needed to increase the size limit, because it was exceeding by 151 B:

✔ Adding to empty webpack project

  Package size limit has exceeded by 151 B
  Size limit: 1.15 kB
  Size:       1.3 kB  with all dependencies, minified and gzipped

Copy link
Collaborator

@hartzis hartzis left a comment

Choose a reason for hiding this comment

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

If that as suggestion works we can probably reduce the size limit again 😸

I can give this a whirl when i do some swipeable work next week too.

src/index.ts Outdated
const delta =
typeof props.delta === "number"
? props.delta
: props.delta[uncapitalizeDirection(dir)];
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 tried a few approaches to trim the size up a bit and avoid the uncapitalizeDirection method. I think we can utilize as here.

Suggested change
: props.delta[uncapitalizeDirection(dir)];
: props.delta[dir.toLowerCase() as Uncapitalize<SwipeDirections>];

Copy link
Contributor Author

@macabeus macabeus Jul 30, 2021

Choose a reason for hiding this comment

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

Even following this approach, it extends the size limit:

  Package size limit has exceeded by 114 B
  Size limit: 1.15 kB
  Size:       1.26 kB with all dependencies, minified and gzipped

Copy link
Collaborator

Choose a reason for hiding this comment

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

😞 that's crazy. It must be something introduced with the bundler upgrade. Quite annoying, i'll have to dig in to the outputs and see if we can figure this discrepancy out.

Also posted this typescript issue to my colleagues and they had a couple other approaches to consider that we can look at.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A +10% increase in size is not a lot but it is also sort of large when your size is already really small. I think we can find the culprit and keep the size down, will just take a bit of investigation that I can take on hopefully this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hartzis I created a diff comparting the output from this branch and from the main, and I got it:

2a3,24
> function _interopNamespace(e) {
>   if (e && e.__esModule) return e;
>   var n = Object.create(null);
>   if (e) {
>     Object.keys(e).forEach(function (k) {
>       if (k !== 'default') {
>         var d = Object.getOwnPropertyDescriptor(e, k);
>         Object.defineProperty(n, k, d.get ? d : {
>           enumerable: true,
>           get: function () {
>             return e[k];
>           }
>         });
>       }
>     });
>   }
>   n['default'] = e;
>   return n;
> }
>
> var React__namespace = /*#__PURE__*/_interopNamespace(React);
>
114c136,137
<       var vxvy = [deltaX / (time || 1), deltaY / (time || 1)]; // if swipe is under delta and we have not started to track a swipe: skip update
---
>       var vxvy = [deltaX / (time || 1), deltaY / (time || 1)];
>       var dir = getDirection(absX, absY, deltaX, deltaY); // if swipe is under delta and we have not started to track a swipe: skip update
116,117c139,140
<       if (absX < props.delta && absY < props.delta && !state.swiping) return state;
<       var dir = getDirection(absX, absY, deltaX, deltaY);
---
>       var delta = typeof props.delta === "number" ? props.delta : props.delta[dir.toLowerCase()];
>       if (absX < delta && absY < delta && !state.swiping) return state;
280,281c303,304
<   var transientState = React.useRef(_extends({}, initialState));
<   var transientProps = React.useRef(_extends({}, defaultProps));
---
>   var transientState = React__namespace.useRef(_extends({}, initialState));
>   var transientProps = React__namespace.useRef(_extends({}, defaultProps));
284c307
<   var _React$useMemo = React.useMemo(function () {
---
>   var _React$useMemo = React__namespace.useMemo(function () {

In short, the new microbundle version injects the function _interopNamespace to be used with React calls.
It comes from Rollup, and we can remove it by disabling the option output.interop, but I'm not sure that it's really good to disable it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@macabeus You rock! amazing find!

thank you for the link to the rollup option, it looks like they are going to be changing output.interop to default to auto so i'm curious if we can try that first. If that doesn't work, maybe we roll back upgrading microbundle and circumvent the need for the typescript updates for now?

Copy link
Collaborator

@hartzis hartzis Aug 13, 2021

Choose a reason for hiding this comment

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

Oka oka.. quick update. TLDR, moving forward with this PR as is for now.

microbundle does not give us access to the rollup config, for good reason, so we're unable to edit the output.interop value.

I'm going to make an issue, #267, that will tackle removing microbundle in the future and use something else, who knows what. I will probably miss the convenience of microbundle but this project's output is so small I think we should have finer grained control over what is output during compile to help manage its file size.

🎉 side note, i think i found a way to "counter" the increase by removing comments from the build via tsc option removeComments

@macabeus macabeus requested a review from hartzis July 30, 2021 16:42
@@ -351,6 +351,81 @@ describe("useSwipeable", () => {
expect(onSwipedRight).not.toHaveBeenCalled();
});

it("allows different delta for each side", () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏 🎉 Thank you for the test!

@@ -40,8 +40,11 @@ export type SwipeableCallbacks = {
};

// Configuration Options
export type ConfigurationOptionDelta =
| number
| { [key in Uncapitalize<SwipeDirections>]: number };
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 🤔 going to take a look at making each direction optional and defaulting to the defaultProps.delta value if not present

@hartzis hartzis merged commit 46dd4a9 into FormidableLabs:main Aug 13, 2021
This was referenced Feb 21, 2022
Merged
@github-actions github-actions bot mentioned this pull request May 18, 2023
@paulmarsicloud paulmarsicloud mentioned this pull request Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: different delta for each side
3 participants