Visual design: Privacy Shield #6000
Conversation
malmstein
force-pushed
the
feature/david/04-29-visual_design_privacy_shield
branch
from
a8156fa to
f4133d0
Compare
app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt
Outdated
Show resolved
Hide resolved
| private fun shouldShowUpdatedPrivacyShield(navigationBarEnabled: Boolean): Boolean { | ||
| return senseOfProtectionExperiment.shouldShowNewPrivacyShield() || navigationBarEnabled | ||
| } |
There was a problem hiding this comment.
I'm not a fan of logic like this baked into the view (mainly for clarity and ease of testing reasons) - we make one if-check here and then another in BrowserLottieTrackersAnimatorHelper.kt to finally select the animation.
What do you think about turning OmnibarLayoutViewModel.LeadingIconState into a sealed class? That way, the view model could provide something like OmnibarLayoutViewModel.LeadingIconState.PRIVACY_SHIELD.resourceType, where resourceType defines which animation to use (out of production, sense of protection, or the new visual update).
I'm not going to block the PR over this, but if you agree this is a worthwhile improvement, it’d be great to incorporate here or as a follow up.
There was a problem hiding this comment.
I don’t like this logic either, and ideally we’d refactor BrowserLottieTrackersAnimatorHelper.kt but that’s not something I want to do now.
The problem is that now we have 3 different tracker animations and 2 of them have different layouts. This is far from optimal, but it solves the issue now knowing that this will be cleaned up once the experiments finish.
| trackers == 1 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_1_visual_updates else R.raw.dark_trackers_1_visual_updates | ||
| trackers == 2 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_2_visual_updates else R.raw.dark_trackers_2_visual_updates | ||
| trackers >= 3 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_visual_updates else R.raw.dark_trackers_visual_updates | ||
| else -> TODO() |
There was a problem hiding this comment.
Missing case to handle (or not, but let's clean up the TODO()).
There was a problem hiding this comment.
This one still needs addressing in some way.
There was a problem hiding this comment.
will show a default and be done with it
app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt
Outdated
Show resolved
Hide resolved
| trackers == 1 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_1_visual_updates else R.raw.dark_trackers_1_visual_updates | ||
| trackers == 2 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_2_visual_updates else R.raw.dark_trackers_2_visual_updates | ||
| trackers >= 3 -> if (theme.isLightModeEnabled()) R.raw.light_trackers_visual_updates else R.raw.dark_trackers_visual_updates | ||
| else -> TODO() |
There was a problem hiding this comment.
This one still needs addressing in some way.
malmstein
force-pushed
the
feature/david/04-29-visual_design_privacy_shield
branch
from
e6ec3d6 to
bfba565
Compare
Task/Issue URL: https://app.asana.com/1/137249556945/project/1174433894299346/task/1210067401123377
Description
Steps to test this PR
Updates states