For #7698: Adds search back button animation #7840

sblatz · 2020-01-21T21:54:52Z

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

codecov-io · 2020-01-21T22:15:28Z

Codecov Report

Merging #7840 into master will decrease coverage by 0.01%.
The diff coverage is 8.69%.

@@             Coverage Diff              @@
##             master    #7840      +/-   ##
============================================
- Coverage     19.18%   19.16%   -0.02%     
  Complexity      451      451              
============================================
  Files           301      301              
  Lines         11639    11659      +20     
  Branches       1577     1579       +2     
============================================
+ Hits           2233     2235       +2     
- Misses         9222     9240      +18     
  Partials        184      184

Impacted Files	Coverage Δ	Complexity Δ
...in/java/org/mozilla/fenix/search/SearchFragment.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
.../java/org/mozilla/fenix/search/SearchController.kt	`79.66% <100%> (+0.35%)`	`0 <0> (ø)`	⬇️
...va/org/mozilla/fenix/search/SearchFragmentStore.kt	`51.11% <12.5%> (-6.79%)`	`1 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b1a673...3f8bd6a. Read the comment docs.

severinrudie

Looks good to me. I think this really cleans up the animation, so I'm happy to see the change. Nice job!

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

severinrudie · 2020-01-21T22:28:56Z

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

@@ -352,7 +357,17 @@ class SearchFragment : Fragment(), UserInteractionHandler {
        }
    }

+    private fun animateBackButtonAway() {
+        val xTranslation =
+            requireView().back_button.width.toFloat() * BACK_BUTTON_ANIMATION_WIDTH_MULTIPLIER


It looks like this constant is to make up for padding + margin? That works, but it's a little bit messy, opens us up to weird bugs if we ever adjust either of those values, and requires a reader to figure out what it's handling. What about something like this:

val backButton = requireView().back_button val xTranslation = with(backButton) { -(width + marginStart + paddingStart).toFloat() }

Seems reasonable to me!

severinrudie · 2020-01-21T22:33:59Z

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

+        view.back_button.apply {
+            setOnClickListener {
+                animateBackButtonAway()
+                findNavController().navigateUp()


I think it's a bad sign that this animates then navigates up, while the ToolbarView animates then calls the interactor, which calls the controller, and... navigates up. This is one of the things our architecture is meant to unify, so let's clean it up while we're here.

If we:

move animateBackButtonAway off of ToolbarView and on to SearchController

replace 205 & 206 in SearchFragment with searchInteractor.onEditingCanceled()

then we have a more reasonable and consistent data flow.

It seems to me that we want back press and back button clicks to behave the same here, so I'm happy to have them call the same interactor method. If that ever changes in the future, we can break them out as needed.

severinrudie

LGTM from the engineering side

severinrudie · 2020-01-22T17:56:57Z

app/src/main/java/org/mozilla/fenix/search/SearchController.kt

@@ -76,6 +76,7 @@ class DefaultSearchController(
    }

    override fun handleEditingCancelled() {
+        store.dispatch(SearchFragmentAction.EditingCancelled)


Nit: there's nothing wrong with this name, but just to align with AC I'd prefer to name it UpdateEditingCanceled.

severinrudie · 2020-01-22T17:57:52Z

app/src/main/java/org/mozilla/fenix/search/SearchFragment.kt

+            -(width + marginStart + paddingStart).toFloat()
+        }
+
+        requireView().back_button


Nit: you can replace this line with backButton

sblatz requested review from brampitoyo and severinrudie January 21, 2020 21:54

sblatz force-pushed the back-button-anim branch 2 times, most recently from d1fa838 to 1bf9958 Compare January 21, 2020 22:04

severinrudie approved these changes Jan 21, 2020

View reviewed changes

sblatz force-pushed the back-button-anim branch 2 times, most recently from fc80cc5 to faca280 Compare January 22, 2020 17:04

For mozilla-mobile#7698: Adds search back button animation

342cda4

sblatz force-pushed the back-button-anim branch from faca280 to 4191987 Compare January 22, 2020 17:53

severinrudie approved these changes Jan 22, 2020

View reviewed changes

sblatz force-pushed the back-button-anim branch 2 times, most recently from 430f8de to d4d2824 Compare January 22, 2020 18:16

Refactor for searchController

3f8bd6a

sblatz force-pushed the back-button-anim branch from d4d2824 to 3f8bd6a Compare January 22, 2020 18:46

sblatz merged commit 20396f7 into mozilla-mobile:master Jan 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #7698: Adds search back button animation #7840

For #7698: Adds search back button animation #7840

sblatz commented Jan 21, 2020

codecov-io commented Jan 21, 2020 •

edited

severinrudie left a comment

severinrudie Jan 21, 2020

sblatz Jan 22, 2020

severinrudie Jan 21, 2020 •

edited

severinrudie left a comment

severinrudie Jan 22, 2020

severinrudie Jan 22, 2020

For #7698: Adds search back button animation #7840

For #7698: Adds search back button animation #7840

Conversation

sblatz commented Jan 21, 2020

Pull Request checklist

After merge

To download an APK when reviewing a PR:

codecov-io commented Jan 21, 2020 • edited

Codecov Report

severinrudie left a comment

Choose a reason for hiding this comment

severinrudie Jan 21, 2020

Choose a reason for hiding this comment

sblatz Jan 22, 2020

Choose a reason for hiding this comment

severinrudie Jan 21, 2020 • edited

Choose a reason for hiding this comment

severinrudie left a comment

Choose a reason for hiding this comment

severinrudie Jan 22, 2020

Choose a reason for hiding this comment

severinrudie Jan 22, 2020

Choose a reason for hiding this comment

codecov-io commented Jan 21, 2020 •

edited

severinrudie Jan 21, 2020 •

edited