-
Notifications
You must be signed in to change notification settings - Fork 4
Expose roundBy5 #31
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
base: master
Are you sure you want to change the base?
Expose roundBy5 #31
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExpose the existing timepicker Sequence diagram for TimePicker hand movement with roundBy5 optionsequenceDiagram
actor User
participant TimePicker
participant State
participant setHand
User->>TimePicker: mousedown on clock face
TimePicker->>State: compute dx, dy from click position
TimePicker->>setHand: setHand(dx, dy, options.roundBy5)
User->>TimePicker: drag (mousemove)
TimePicker->>State: update x, y, moved = true
TimePicker->>setHand: setHand(x, y, options.roundBy5, true)
setHand-->>TimePicker: hand position (snapped if roundBy5)
TimePicker-->>User: update clock UI
Class diagram for updated TimepickerOptions and TimePicker interactionsclassDiagram
class TimepickerOptions {
+boolean~optional~ autoClose
+boolean~optional~ twelveHour
+boolean~optional~ vibrate
+boolean~optional~ roundBy5
+function~optional~ onOpen()
+function~optional~ onOpenStart()
+function~optional~ onOpenEnd()
}
class DefaultTimepickerOptions {
+boolean autoClose = false
+boolean twelveHour = true
+boolean vibrate = true
+boolean roundBy5 = false
+function onOpen()
+function onOpenStart()
+function onOpenEnd()
}
class TimePicker {
+TimepickerOptions options
+State state
+handleMousedown(event)
+handleMousemove(event)
}
class State {
+number dx
+number dy
+number x0
+number y0
+boolean moved
}
class setHand {
+setHand(dx, dy, roundBy5, moved?)
}
DefaultTimepickerOptions --> TimepickerOptions : implements
TimePicker --> TimepickerOptions : uses options
TimePicker --> State : maintains
TimePicker --> setHand : calls
TimepickerOptions --> setHand : provides roundBy5
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- When passing
options.roundBy5intosetHand, consider normalizing it with a default (e.g.!!options.roundBy5) in case an options object is constructed without going through thedefaultOptionsmerge path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When passing `options.roundBy5` into `setHand`, consider normalizing it with a default (e.g. `!!options.roundBy5`) in case an options object is constructed without going through the `defaultOptions` merge path.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@erikvullings what do you think about this small improvement? |
There is currently a flag for time pickers called
roundBy5, but it is never used as it is always set tofalse.Since I would like to use it, I propose it is exposed to the developer.