Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Fixes #5323: Show a snackbar while browsing data is deleting. #6105

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

ValentinTimisica
Copy link
Contributor

I set the snackbar length indefinite because it should show while the
browsing data is deleting. After that the snackbar is dismissed.
In BaseBrowserFragment I didn't set an anchor for the snackbar because
browserToolbarView is initialized after DefaultBrowserToolbarController
constructor where I pass the snackbar as a parameter (browserToolbarView
needs browserInteractor who needs browserToolbarController, so I
couldn't change the initialization order). The only solution I found was
to pass the snackbar to DefaultBrowserToolbarController via a setter but
this would bypass our current architecture.


Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks.
  • Tests: This PR includes thorough tests.
  • Screenshots: This PR includes a video showing the snackbar.
  • Accessibility: The code in this PR follows accessibility best practices.

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:

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

Copy link
Contributor

@ekager ekager left a comment

Choose a reason for hiding this comment

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

R+ with one nit about the new string! I think having it display over the toolbar is fine because we're quitting the app anyway and the toolbar isn't usable in this time 🤷‍♀

@@ -738,6 +738,8 @@
<string name="delete_browsing_data_prompt_allow">Delete</string>
<!-- Text for the snackbar confirmation that the data was deleted -->
<string name="preferences_delete_browsing_data_snackbar">Browsing data deleted</string>
<!-- Text for the snackbar to show the user that the deletion of browsing data is in progress -->
<string name="deleting_browsing_data_in_progress">Deleting browsing data…</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want to use &#8230 for the ellipsize here like in onboarding_firefox_account_signing_in

@ekager ekager added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Oct 19, 2019
@codecov-io
Copy link

codecov-io commented Oct 19, 2019

Codecov Report

Merging #6105 into master will decrease coverage by 0.01%.
The diff coverage is 8.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #6105      +/-   ##
===========================================
- Coverage     14.32%   14.3%   -0.02%     
  Complexity      326     326              
===========================================
  Files           261     261              
  Lines         10725   10735      +10     
  Branches       1565    1569       +4     
===========================================
  Hits           1536    1536              
- Misses         9065    9073       +8     
- Partials        124     126       +2
Impacted Files Coverage Δ Complexity Δ
...c/main/java/org/mozilla/fenix/home/HomeFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...fenix/settings/deletebrowsingdata/DeleteAndQuit.kt 50% <0%> (-6.25%) 0 <0> (ø)
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...nix/components/toolbar/BrowserToolbarController.kt 76.85% <100%> (ø) 0 <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 d575c25...ed606ee. Read the comment docs.

…eting.

I set the snackbar length indefinite because it should show while the
browsing data is deleting. After that the snackbar is dismissed.
In BaseBrowserFragment I didn't set an anchor for the snackbar because
browserToolbarView is initialized after DefaultBrowserToolbarController
constructor where I pass the snackbar as a parameter (browserToolbarView
needs browserInteractor who needs browserToolbarController, so I
couldn't change the initialization order). The only solution I found was
to pass the snackbar to DefaultBrowserToolbarController via a setter but
this would bypass our current architecture.
@boek boek merged commit 485ccba into mozilla-mobile:master Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants