Allow scroll by the amount of days setted in NumberOfVisibleDays#68
Allow scroll by the amount of days setted in NumberOfVisibleDays#68SkyleKayma wants to merge 2 commits intoQuivr:developfrom SkyleKayma:CustomScrollableDaysAmount
Conversation
| // Reset scrolling and fling direction. | ||
| mCurrentScrollDirection = mCurrentFlingDirection = Direction.NONE; | ||
| }else { | ||
| sizeOfScrollableView = (mWidthPerDay + mColumnGap) * getNumberOfVisibleDays(); |
There was a problem hiding this comment.
There is some code duplication in the if part and in the else part. It would be best, if that part can be executed outside of the if else. Especially the mScroller.startScroll etc
There was a problem hiding this comment.
I could move outside :
// Reset scrolling and fling direction.
mCurrentScrollDirection = mCurrentFlingDirection = Direction.NONE;
But the rest of the code is not really a duplicate part :/
There was a problem hiding this comment.
there are a lot of lines that are equal but have different values for the parameters, you can probably create variables for those parameters, set these variables in the if and else and place a lot of code after the if else
| //Scroll by amount of days desired functionnality | ||
| private boolean mIsCustomScrollableAmountsOfDaysEnabled = false; | ||
| private float startOriginForScroll = 0; | ||
| private float sizeOfScrollableView; |
There was a problem hiding this comment.
Don't create a field for this, but use a method.
|
|
||
| //Scroll by amount of days desired functionnality | ||
| private boolean mIsCustomScrollableAmountsOfDaysEnabled = false; | ||
| private float startOriginForScroll = 0; |
There was a problem hiding this comment.
What is the difference and relation with mCurrentOrigin.x ?
There was a problem hiding this comment.
The current origin.x is updated when I started to scroll.
For exemple I start at 0 on x axis.
I start a scroll and now I'am at -200 (Scroll Direction Left)
The mCurrentOrigin.x value is -200.0, and startOriginForScroll will stay at 0. I've done that because I have added in my own repo the functionnality "safe scrolling" that let you go back to the latest valid position you had.
There was a problem hiding this comment.
ok, but wouldn't it be possible without this variable ? If it is possible, please remove this extra variable
| private AddEventClickListener mAddEventClickListener; | ||
|
|
||
| //Scroll by amount of days desired functionnality | ||
| private boolean mIsCustomScrollableAmountsOfDaysEnabled = false; |
There was a problem hiding this comment.
Should have a better name, mIsScrollByAmountOfVisibleDaysEnabled for example
There was a problem hiding this comment.
Ok ( Mixing bad english and bad imagination created that x) )
| } | ||
| } | ||
|
|
||
| public void setCustomScrolableDaysEnabled(boolean isEnabled){ |
There was a problem hiding this comment.
The name if this setter has to change, according to the new name for the field.
jhoobergs
left a comment
jhoobergs
left a comment
There was a problem hiding this comment.
Looks good, can you answer my previous questions ?
|
I've answered to you, but don't know why is "pending". |
2 participants
TestVideo.zip
It allows the scroll by NumberOfVisibleDays set just before