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

Allow scroll by the amount of days setted in NumberOfVisibleDays #68

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Allow scroll by the amount of days setted in NumberOfVisibleDays #68

wants to merge 2 commits into from

Conversation

SkyleKayma
Copy link

@SkyleKayma SkyleKayma commented Jan 7, 2018

TestVideo.zip
It allows the scroll by NumberOfVisibleDays set just before

// Reset scrolling and fling direction.
mCurrentScrollDirection = mCurrentFlingDirection = Direction.NONE;
}else {
sizeOfScrollableView = (mWidthPerDay + mColumnGap) * getNumberOfVisibleDays();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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 :/

Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

Don't create a field for this, but use a method.

Copy link
Author

Choose a reason for hiding this comment

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

Ok

@@ -179,6 +179,11 @@
private ScrollListener mScrollListener;
private AddEventClickListener mAddEventClickListener;

//Scroll by amount of days desired functionnality
private boolean mIsCustomScrollableAmountsOfDaysEnabled = false;
private float startOriginForScroll = 0;
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference and relation with mCurrentOrigin.x ?

Copy link
Author

@SkyleKayma SkyleKayma Jan 7, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

ok, but wouldn't it be possible without this variable ? If it is possible, please remove this extra variable

@@ -179,6 +179,11 @@
private ScrollListener mScrollListener;
private AddEventClickListener mAddEventClickListener;

//Scroll by amount of days desired functionnality
private boolean mIsCustomScrollableAmountsOfDaysEnabled = false;
Copy link
Member

Choose a reason for hiding this comment

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

Should have a better name, mIsScrollByAmountOfVisibleDaysEnabled for example

Copy link
Author

@SkyleKayma SkyleKayma Jan 7, 2018

Choose a reason for hiding this comment

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

Ok ( Mixing bad english and bad imagination created that x) )

@@ -2488,6 +2528,10 @@ private boolean forceFinishScroll() {
}
}

public void setCustomScrolableDaysEnabled(boolean isEnabled){
Copy link
Member

Choose a reason for hiding this comment

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

The name if this setter has to change, according to the new name for the field.

Copy link
Author

Choose a reason for hiding this comment

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

Ok

Copy link
Member

@jhoobergs jhoobergs left a comment

Choose a reason for hiding this comment

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

Looks good, can you answer my previous questions ?

@SkyleKayma
Copy link
Author

SkyleKayma commented Jan 9, 2018

I've answered to you, but don't know why is "pending".
git
I will make the change you want in the code when I will have some time. It's ok, i'll make this functionnality until the end. ;)
(I find a "bug" (maybe not one) in goToHour in the stable lib actually, I will make an issue later, but this takes me some time to debug.)

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