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: ability to modify autoScroller config options #434

Merged
merged 14 commits into from
Nov 26, 2022

Conversation

spectraldoy
Copy link
Contributor

@spectraldoy spectraldoy commented Nov 14, 2022

Following #431, I've made some edits to DragDropContext, added an autoScrollOptions prop that allows the user significantly more control over the behavior of auto-scroll. This prop takes as input an object that follows the following interface (src/state/auto-scroller/fluid-scroller/config/autoscroll-config-types.ts, adapted from src/state/auto-scroller/fluid-scroller/config.ts):

interface PartialAutoScrollConfig {
  // percentage distance form edge of container
  startFromPercentage?: number;
  maxScrollAtPercentage?: number;
  // pixels per frame
  maxPixelScroll?: number;
  // A function used to ease a percentage value
  ease?: (percentage: number) => number;
  durationDampening?: {
    // ms: how long to dampen the speed of an auto scroll from the start of a drag
    stopDampeningAt?: number;
    // ms: when to start accelerating the reduction of duration dampening
    accelerateAt?: number;
  };
  // whether or not autoscroll should be turned off entirely
  disabled?: boolean;
}

Requesting this PR be reviewed. Tagging @Xhale1 as they participated in the above-referenced issue.

Notes:

  • Have not yet written any tests. I need to understand how to properly build this project so that it can be imported as a library in another program. The way I tested and my failed attempts at building are detailed in this comment. Would very much appreciate help, perhaps through a step-by-step guide to building the library.
  • With the changes introduced, I get "unexpected store change" warning errors in the console. Not sure what I need to do to resolve these, so I thought I should ask for help.
  • Another TODO I would like to do in the coming days is update the auto-scrolling.md doc.

@vercel
Copy link

vercel bot commented Nov 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dnd ✅ Ready (Inspect) Visit Preview Nov 22, 2022 at 7:01AM (UTC)

@Xhale1
Copy link
Collaborator

Xhale1 commented Nov 14, 2022

This library makes heavy use of memorization, I imagine something here (likely the new props) is breaking memorization. It should be a pretty easy fix, though it might take some understanding of the library.

I'll take a look in the morning (it's 3am here, oops).

As for building this package, I great way to test things is with storybook :)

npm run storybook will build a website locally where you can test a bunch of example uses. It'd be awesome if you added a story demonstrating this new feature. I don't mind looking into that more tomorrow as well.

@spectraldoy
Copy link
Contributor Author

Haha no worries, I'm in PST too! I'll look into storybook. Thanks!

@spectraldoy
Copy link
Contributor Author

spectraldoy commented Nov 15, 2022

Update: storybook is awesome! I've tested my code with it, but still haven't written any formal tests that would go in the test directory. Regardless, I was able to test my changes without any errors except that warning message ("unexpected store change", which is coming from /src/view/drag-drop-context/app.tsx). I've added a story that allows the user to play with all the options I've added for configuring autoScroll, and I've updated the docs. Let me know how to proceed from here. Thanks!

@spectraldoy spectraldoy changed the title feat: Issue #431 More control over autoScroll within DragDropCo… feat: issue #431 More control over autoScroll within DragDropContext Nov 15, 2022
@spectraldoy spectraldoy changed the title feat: issue #431 More control over autoScroll within DragDropContext feat: issue #431 more control over autoScroll within DragDropContext Nov 15, 2022
@spectraldoy
Copy link
Contributor Author

@Xhale1 sorry to ping, but just wanted to get an update on this. Was hoping I could get this out of the way quickly.

@Xhale1
Copy link
Collaborator

Xhale1 commented Nov 18, 2022

Hey, sorry for dropping the ball on this.

There were 11 failing tests due to autoScrollOptions being required but not set, just pushed a commit to fix those and now everything passes.

There are a few ESLint errors which need to get resolved (working on those now).

@100terres we should probably write some more better documentation onsetting up your editor to find ESLint and Typescript errors :)

@spectraldoy
Copy link
Contributor Author

No worries, and thanks so much! I really appreciate your working with me on this!

@Xhale1
Copy link
Collaborator

Xhale1 commented Nov 19, 2022

Alright, I cleaned up the code a bit and, fixed the remaining ESLint errors, and fixed the store ref unexpectedly changing. All tests are passing and the storybook is working great!

I think we're good to merge! Just gonna ping @100terres to take a second look just in case and then I'm good to do a release.

@spectraldoy
Copy link
Contributor Author

spectraldoy commented Nov 19, 2022

Wahoo! The cleaned up code looks much more elegant. Thanks so much for taking the time to write the unit tests and refactor everything. I really appreciate it.

Also I noticed a typo in autoscroll-setter.tsx that was causing errors in the eslint check. Hopefully everything passes now.

@spectraldoy
Copy link
Contributor Author

@Xhale1 @100terres just bumping this because looks like the tests are passing and this could be closed soon. Thanks!

@100terres
Copy link
Collaborator

@spectraldoy I'll try to have a look it tonight or tomorrow night (EST)!

@100terres
Copy link
Collaborator

@spectraldoy great job! I'll do a second round of review tomorrow! Thank you for opening this pull request 🚀

@100terres 100terres self-assigned this Nov 23, 2022
@100terres 100terres added the enhancement New feature or request label Nov 23, 2022
@spectraldoy
Copy link
Contributor Author

spectraldoy commented Nov 23, 2022

Thank you @100terres but I only introduced the controls, @Xhale1 really was the one who made it elegant and efficient, and squashed my bugs 😅. I'm glad you like this though, because I really needed it lol.

Copy link
Collaborator

@Xhale1 Xhale1 left a comment

Choose a reason for hiding this comment

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

LGTM

@Xhale1 Xhale1 changed the title feat: issue #431 more control over autoScroll within DragDropContext feat: ability to modify autoScroller config options Nov 25, 2022
@Xhale1 Xhale1 merged commit 8277937 into hello-pangea:main Nov 26, 2022
@spectraldoy
Copy link
Contributor Author

Thank you!

@100terres 100terres removed their request for review February 20, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants