Feature/david/pull to refresh #750
Conversation
malmstein
requested review from
CDRussell,
cmonfortep,
marcosholgado and
subsymbolic
as code owners
| @@ -19,8 +19,7 @@ | |||
| xmlns:tools="http://schemas.android.com/tools" | |||
| android:id="@+id/rootView" | |||
| android:layout_width="match_parent" | |||
| android:layout_height="match_parent" | |||
| app:layout_scrollFlags="scroll|enterAlways"> | |||
There was a problem hiding this comment.
these flags don't affect the behaviour, it's set manually by OmnibarScrolling
| @@ -23,20 +23,37 @@ import android.annotation.SuppressLint | |||
| import android.app.Activity.RESULT_OK | |||
| import android.app.ActivityOptions | |||
| import android.appwidget.AppWidgetManager | |||
| import android.content.* | |||
| import android.content.ClipData | |||
There was a problem hiding this comment.
There shouldn't be all of these changes to the import statements; when i format this file it reverts them back again and this is going to create lots of noisy PRs.
Can you check your code formatting settings to ensure they match the team settings? I can work with you offline if they're out of sync.
There was a problem hiding this comment.
I'm afraid those guidelines don't mention anything about Imports. On the other hand, Unused imports is a well know PMD warning (https://pmd.github.io/latest/pmd_rules_java_bestpractices.html#unusedimports) Wildcard imports are unnecessary, why is that better?
There was a problem hiding this comment.
Wildcard imports are unnecessary, why is that better?
It's not a case of which is better; it's about not generating a ton of changes every time the code is touched by different people. I've no strong opinions on whether to wildcard imports or not, per se, but i've a very strong opinion we all need to be doing the same thing.
There was a problem hiding this comment.
There's something not right with the UX here;
- the swipe gesture is overly sensitive; the slightest scroll down rapidly brings the indicator down fully. It doesn't smoothly correlate with the number of pixels dragged
- once initiated, it can't be cancelled
Compare the UX with Chrome, where the indicator is much smoother and can be cancelled. (attached a video to try demonstrate the difference; first app is chrome then ours. Available here: https://app.asana.com/0/414730916066338/1168653724774521)
| @@ -462,6 +558,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope { | |||
| } | |||
|
|
|||
| fun refresh() { | |||
| swipeContainer.isRefreshing = true | |||
There was a problem hiding this comment.
Do not call this when refresh is triggered by a swipe gesture.
The documentation on SwipeRefreshLayout suggests we shouldn't set this here?
There was a problem hiding this comment.
This is triggered when the button in the Popup Menu is pressed, so it's necessary.
| this.canWebViewScrollUpCallback = canWebViewScrollUpCallback | ||
| } | ||
|
|
||
| override fun canChildScrollUp(): Boolean { |
There was a problem hiding this comment.
Instead of the !!, what about if changed the function like this?
override fun canChildScrollUp(): Boolean {
return canWebViewScrollUpCallback?.canSwipeRefreshChildScrollUp() ?: super.canChildScrollUp()
}
|
Closing this until we agree on UX flow with Design. Thanks for the review @CDRussell! |
Task/Issue URL: https://app.asana.com/0/488551667048375/1168653724774521
Description:
Pull to Refresh is a very common pattern on Android when it comes to refresh the content in screen. This PR enables this mechanism, without removing the current option for refreshing**Steps to test
Screencast: