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 page finishing function when over scrolled to SessionDetailActivity #359

Merged
merged 2 commits into from Feb 18, 2017

Conversation

Projects
None yet
4 participants
@chibatching
Contributor

chibatching commented Feb 17, 2017

Issue

Overview (Required)

  • Finish SessionDetailActivity when page overscrolled

Links

  • none

Screenshot

Before After
@konifar

This comment has been minimized.

Collaborator

konifar commented Feb 17, 2017

Cool 🆒

@konifar

This comment has been minimized.

Collaborator

konifar commented Feb 17, 2017

Interesting implementation 👀
I thought it's better to make custom activity, but your custom CoordinatorLayout also looks good and reasonable. Let me taste this on my device after CI success 👍



@BindingMethods({
@BindingMethod(type = OverScrollLayout.class, attribute = "onOverScroll", method = "setOverScrollListener")

This comment has been minimized.

@konifar

konifar Feb 18, 2017

Collaborator

🆒

@konifar

This comment has been minimized.

Collaborator

konifar commented Feb 18, 2017

Sorry to be late review 🙇
I checked on my device. It's really awesome! Thank you so much 👍
LGTM! 💯

@konifar konifar merged commit e470424 into DroidKaigi:master Feb 18, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@konifar konifar removed the in progress label Feb 18, 2017

@chibatching

This comment has been minimized.

Contributor

chibatching commented Feb 19, 2017

Thank you for your review!! 😄

@Override
public void onStopNestedScroll(View target) {
super.onStopNestedScroll(target);
if (Math.abs(getY()) > overScrollThreshold && overScrollListener != null) {

This comment has been minimized.

@jmatsu

jmatsu Feb 20, 2017

Member

overScrollListener != null should be checked first.
Could you please swap?

This comment has been minimized.

@chibatching

chibatching Feb 20, 2017

Contributor

Oh, I see. I'd like to make another PR to fix it.


private int overScrollThreshold;

private PointF originalLocation = new PointF();

This comment has been minimized.

@jmatsu

jmatsu Feb 20, 2017

Member

Rect is better, I think. Then, Rect#getHeight can be used instead of originalHeight.

})
public class OverScrollLayout extends CoordinatorLayout {

private static final float OVER_SCROLL_THRESHOLD = 0.20f;

This comment has been minimized.

@jmatsu

jmatsu Feb 20, 2017

Member

This variable is a scale or ratio to calculate threshold?
Can you add any suitable suffix?

@@ -157,6 +157,10 @@ public void onClickFab(@SuppressWarnings("unused") View view) {
}
}

public void onOverScroll() {
callback.onOverScroll();

This comment has been minimized.

@jmatsu

jmatsu Feb 20, 2017

Member

should check null or not.

}

private boolean isAppBarExpanded(@NonNull AppBarLayout appBarLayout) {
return appBarLayout.getHeight() == appBarLayout.getBottom();

This comment has been minimized.

@jmatsu

jmatsu Feb 20, 2017

Member

Does this really work?
View#getHeight() is calculated by View#getBottom() - View#getTop().
Can isAppBarExpanded and View#getTop() == zero be used equally?

This comment has been minimized.

@chibatching

chibatching Feb 20, 2017

Contributor

View#getHeight() is calculated by View#getBottom() - View#getTop()

I didn't know this, it may be able to replaced with View#getTop() == zero.
Thanks!

@jmatsu

This comment has been minimized.

Member

jmatsu commented Feb 20, 2017

Oops, I didn't notice this PR was merged 😇
Sorry, please ignore my reviews.

@chibatching

This comment has been minimized.

Contributor

chibatching commented Feb 20, 2017

I'd like to make another PR to fix your comment! 👍

@konifar

This comment has been minimized.

Collaborator

konifar commented Feb 20, 2017

@jmatsu Thanks for great review 😄
@chibatching Appreciate you. Looking forward to next PR 👍

@chibatching chibatching deleted the chibatching:add_overscroll_dismiss_function branch Feb 20, 2017

@chibatching chibatching referenced this pull request Feb 20, 2017

Merged

Fix some PR comment #372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment