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

add the ability to add intervals to the timer picker #39

Closed
wants to merge 7 commits into from

Conversation

OmerToledo
Copy link

No description provided.

@OmerToledo
Copy link
Author

OmerToledo commented Sep 16, 2024

@troberts-28
i still have 2 issues
take a look at the video :)

  1. it doesnt reach the 60
  2. when i scroll sometimes there is a weird jumping in the ui
    any idea what causing
    this bugs ?

btw, im not sure its happing when infinite scroll is disabled

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-16.at.16.07.09.mp4

@troberts-28
Copy link
Owner

Hey @OmerToledo 👋

Thanks for the PR! The issues are related to this: #33.

Essentially, the infinite scroll works by auto-scrolling further up the list when you near the start/end of the list. There is a very slight flicker when it does that and I haven't been able to figure out a way of avoiding that completely. To avoid the flicker being noticeable to the user, the numbers are repeated a given number of times (so it does not have to auto-scroll as often). You'll find props on the TimerPicker component to modify the number of repetitions for each picker. The right approach would be to modify those defaults based on the interval (i.e. the length of the list).

Does that make sense?

@troberts-28
Copy link
Owner

Hey @OmerToledo

Sorry, have really slept on this! Will take a look at this this week and get it merged.

@troberts-28
Copy link
Owner

This has now been merged into v2.0.0. Thanks @OmerToledo!

Just a minor pointer for future work - use more descriptive commit messages to make it easier for a reviewer to understand your work.

@OmerToledo
Copy link
Author

hi no problem!
thanks for the engagement :)

@OmerToledo
Copy link
Author

also, i saw that because of the facts that you didnt merged my changed directly from here, I'm not a contributor which can be nice, but of course its up to your decision :)

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.

2 participants