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.

Sign up for GitHub

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

Jump to bottom

Tab Switcher Animation: Add show/hide behaviour #5729

Merged
merged 24 commits into from
Mar 17, 2025
Merged

Tab Switcher Animation: Add show/hide behaviour #5729

merged 24 commits into from
Mar 17, 2025

Conversation

mikescamell
Copy link
Contributor

@mikescamell mikescamell commented Mar 4, 2025 •
edited by 0nko
Loading

Task/Issue URL: https://app.asana.com/0/1207908166761516/1209368500700767

Description

Added show/hide logic for the animated tile. The tile appears when users have blocked at least 10 trackers and have 2 or more tabs open.

Once visible the tile does not get removed unless:

  • The user dismisses it
  • The user clears data either manually or automatically through data clearing settings

Once dismissed, the tile will not reappear until explicitly re-enabled or reset through dev settings.

Steps to test this PR

Pre-requisites: tabSwitcherAnimation feature toggle is turned on

Tab Switcher Animated Tile Showing

  • Open the TabSwitcher
  • The animated tile should not should be visible
  • Open a website in the new tab (bbc.com for example)
  • Add another tab and open another website (cnn.com for example)
  • Open the TabSwitcher
  • The Animation Tile should be visible
  • If it's not visible it is probably because you do not have enough trackers. Open another website in either one of the tabs (ign.com for example)
  • The Animation Tile should now be visible and display the tracker count

Tab Switcher Animated Tile Stays Visible Once Seen

  • Delete one of the tabs
  • The animated tile should still be visible
  • Leave the Tab Switcher
  • Re-enter
  • The animated tile should still be visible

Tab Switcher Animated Tile Dismissal

  • Dismiss the animated tile
  • Leave the Tab Switcher
  • Re-enter
  • The animated tile should not be visible

Tab Switcher Animated Tile Clearing Data

  • Go to Dev Settings
  • Click "Reset TabSwitcher Animated Tile"
  • Open the TabSwitcher
  • Open a new tab and load a website
  • Go back to the TabSwitcher
  • The animated tile should be visible
  • Open Settings
  • Open Data Clearing
  • Click "Automatically Clear Data"
  • Select "App exit only"
  • Leave Settings
  • Exit the app
  • Swipe the app away from recents
  • Open the app
  • Open the Tab Switcher
  • The animated tile and all tabs should no longer exist

Tab Switcher Animated Tile Show Again After Clearing Data

  • Repeat steps for Tab Switcher Animated Tile Showing
  • Animated tile should show again

Tab Switcher Animated Tile Close All Tabs

  • Open the TabSwitcher
  • The animated tile should be visible
  • Press the overflow menu button
  • Click "Close All Tabs"
  • Click "Close" in the dialog
  • The animated tile and all tabs should no longer exist

Tab Switcher Animated Tile Show Again After Close Tabs

  • Repeat steps for Tab Switcher Animated Tile Showing
  • Animated tile should show again

Demo

Screen_recording_20250307_190356.mp4

This was referenced Mar 4, 2025
Merged
Merged
Merged
@mikescamell Graphite App
Copy link
Contributor Author

mikescamell commented Mar 4, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@mikescamell mikescamell changed the title Tab Switcher Animation: Add dismissal behaviour Tab Switcher Animation: Add show/hide behaviour Mar 4, 2025
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from c08e5e9 to 684efba Compare March 6, 2025 17:16
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/translations branch 2 times, most recently from 6b63d29 to a5c2d0d Compare March 6, 2025 18:07
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch 2 times, most recently from 6f1378c to cf82097 Compare March 7, 2025 07:50
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/translations branch from a5c2d0d to faeee79 Compare March 7, 2025 07:50
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from cf82097 to 2603460 Compare March 7, 2025 16:48
@mikescamell mikescamell changed the base branch from feature/mike/tab-switcher-tile-animation/translations to graphite-base/5729 March 11, 2025 08:52
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from 8f336a1 to 47f8a30 Compare March 11, 2025 17:47
@mikescamell mikescamell changed the base branch from graphite-base/5729 to feature/mike/tab-switcher-tile-animation/translations March 11, 2025 17:47
@mikescamell mikescamell marked this pull request as ready for review March 11, 2025 19:51
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/translations branch from 0bca9d3 to a584d32 Compare March 12, 2025 10:19
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from 47f8a30 to 42edf95 Compare March 12, 2025 10:19
@0nko 0nko self-assigned this Mar 12, 2025
0nko
0nko approved these changes Mar 12, 2025
View reviewed changes
Copy link
Member

@0nko 0nko left a comment

Choose a reason for hiding this comment

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

Works as expected, nice job! 🎖️

Left a comment in the code but I'll let you decide what to do.

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherAdapter.kt
@@ -77,6 +77,7 @@ class TabSwitcherAdapter(
private val list = mutableListOf<TabSwitcherItem>()
private var isDragging: Boolean = false
private var layoutType: LayoutType = LayoutType.GRID
private var onAnimationTileCloseClickListener: (() -> Unit)? = null
Copy link
Member

Choose a reason for hiding this comment

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

There's already a TabSwitcherListener supplied in the constructor that contains a couple of callbacks, WDYT about moving it there?

app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherTileAnimationMonitor.kt
private const val MINIMUM_TRACKER_COUNT = 10
private const val MINIMUM_TAB_COUNT = 2

class TabSwitcherTileAnimationMonitor @Inject constructor(
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from 42edf95 to 438730e Compare March 12, 2025 15:08
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/translations branch from ee7bb0e to a66cf46 Compare March 12, 2025 15:08
@cmonfortep cmonfortep requested a review from Copilot March 17, 2025 09:36
Copilot
Copilot AI reviewed Mar 17, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.

@mikescamell
Add click listener for tracker animation tile close action
b2c3138
@mikescamell
Remove redundant qualifier names
b20782c
@mikescamell
formatting
01f2b25
@mikescamell
Fix tests
1ae4adc
@mikescamell
Add tracker animation tile conditionally based on tracker count
e28b958 
We only show if you have 10 trackers or more

Next we'll check the tab count
@mikescamell
Update tracker animation tile dismissal logic and conditionally inclu…
eedce31 
…de based on tab count

Only show if we have 2 or more tabs open

Next we'll extract this logic out
@mikescamell
Move Tracker Animation Tile logic to TabSwitcherTileAnimationMonitor
d48ee6a 
The logic for determining the visibility of the Tracker Animation Tile has been moved to a new `TabSwitcherTileAnimationMonitor` class.

This includes:
- Determining the tracker count in the last 7 days.
- Determining the number of open tabs.
- Checking if the animation tile is dismissed.
- Defining the minimum requirements for displaying the tile (minimum tracker count and minimum tab count).

The `WebTrackersBlockedAppRepository` has been updated to have a new public method to retrieve the number of tracker count for last 7 days.

The `TabSwitcherViewModel` was refactored to use the new `TabSwitcherTileAnimationMonitor` and the `observeAnimationTileVisibility` function.

The `createTrackerAnimationTile` and `getTrackerCountForLast7Days` were deleted.
@mikescamell
Introduce Animation Tile Seen State
d8e8fab 
- Adds a new state to track if the animation tile has been seen.
- Modifies the logic for displaying the animation tile to include the new state.
- The animation tile will show if NOT dismissed but already seen, even if we have less than 2 tabs
@mikescamell
Add animation tile visibility logic and tests
8af5cd1
@mikescamell
Add tests for Animation Tile visibility in TabSwitcherViewModel
93f1274
@mikescamell
Formatting
f5ebb86
@mikescamell
Use new gradient background
23ed0b2
@mikescamell
Ensure list view text matches regular tab text
fdcacc1 
We want the text in the Animation tile to match the text in the tabs.
@mikescamell
Reset animation when "Close All Tabs" is pressed
c6ce1ef 
If the user presses close all tabs we reset the tile and it will show again if 2 tabs are visible and more than 10 trackers
@mikescamell
Reset seen animation tile state on clear data
ff33b1b 
We've decided to reset the tile if a user clears data as it's done so often that they might not see the tile much.

We do not permanently dismiss the tile unless the user clicks the close button themselves
@mikescamell
Reset setAnimationTileSeen in DevSettings
8c12631 
Makes it much easier to test. Renamed the text to make it clear we're resetting as the new logic for showing/hiding determines whether you will see it.
@mikescamell
Fix tests
a5c664a
@mikescamell
Add dark ripple background for close button and update layout
e14ac01 
In dark mode we end up with a white button
@mikescamell
Switch to new shield animation with gradient
22461c4 
Something wen wrong with the previous Lottie export for the animation and we were missing the shield gradient. This new animation now has that missing gradient.
@mikescamell
Prevent setting animation tile seen if the first item is already a Tr…
fa14769 
…ackerAnimationTile

This prevents a race condition when close all tabs is called and stops the animated tile staying visible due to the reactive changes that stem from setting the tab as seen/unseen.

We need this as a result of recent behaviour changes to close all tabs/fire button scenarios where we do NOT want the animated tile permanently dismissed. Only if a user presses the close button.
@mikescamell
Adjust animation speeds
4d29f15 
While lower and upper thresholds are now the same I'm leaving them as separate as there might be changes come ship review
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/translations branch from 52a92a5 to 406b808 Compare March 17, 2025 17:28
@mikescamell mikescamell force-pushed the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch from 438730e to 4d29f15 Compare March 17, 2025 17:28
@mikescamell mikescamell mentioned this pull request Mar 17, 2025
Merged
20 tasks
@mikescamell mikescamell merged commit 0b63a80 into feature/mike/tab-switcher-tile-animation/translations Mar 17, 2025
4 of 5 checks passed
@mikescamell mikescamell deleted the feature/mike/tab-switcher-tile-animation/add-show-hide-logic branch March 17, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@0nko 0nko 0nko approved these changes

@malmstein malmstein Awaiting requested review from malmstein

@nalcalag nalcalag Awaiting requested review from nalcalag

Assignees

@0nko 0nko

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mikescamell @0nko