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

Create toast UI #5187

Merged
merged 11 commits into from
Nov 7, 2024
Merged

Create toast UI #5187

merged 11 commits into from
Nov 7, 2024

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Oct 24, 2024 •
edited
Loading

Task/Issue URL: https://app.asana.com/0/1204920898013511/1208572901396840/f

Description

Steps to test this PR

Feature 1

  • Fresh install
  • Complete onboarding
  • Go to FF inventory and enable brokenSitePrompt
  • Restart the app
  • Load a site
  • Notice the prompt appears
  • Click on report broken site
  • Check the report broken site form is launched
  • Close and reload a site again
  • Check the prompt appears again
  • Press dismiss
  • Check the toast is dismissed

UI changes

Screenshot_20241106_153430

@CrisBarreiro Graphite App
Copy link
Contributor Author

CrisBarreiro commented Oct 24, 2024 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

Join @CrisBarreiro and the rest of your teammates on Graphite Graphite

This was referenced Oct 24, 2024
Draft
Merged
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-ff branch from c97b284 to d7377d2 Compare October 28, 2024 13:38
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch from df513f9 to 9d2ea88 Compare October 28, 2024 13:38
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-ff branch from d7377d2 to 54fb184 Compare October 29, 2024 09:33
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch from 9d2ea88 to 6540513 Compare October 29, 2024 09:33
@CrisBarreiro CrisBarreiro mentioned this pull request Oct 30, 2024
Merged
8 tasks
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-ff branch from 54fb184 to d3c62f3 Compare October 31, 2024 10:19
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch from 6284500 to e57d6a0 Compare October 31, 2024 10:19
@CrisBarreiro CrisBarreiro marked this pull request as ready for review October 31, 2024 11:36
@CrisBarreiro CrisBarreiro mentioned this pull request Oct 31, 2024
Merged
50 tasks
@CrisBarreiro CrisBarreiro changed the title Create toast UI (WIP) Create toast UI Nov 5, 2024
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-ff branch from d3c62f3 to f4943ba Compare November 5, 2024 11:25
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch from e57d6a0 to b83bc3f Compare November 5, 2024 11:25
@CrisBarreiro CrisBarreiro mentioned this pull request Nov 5, 2024
Merged
2 tasks
lmac012
lmac012 approved these changes Nov 5, 2024
View reviewed changes
Copy link
Contributor

@lmac012 lmac012 left a comment

Choose a reason for hiding this comment

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

LGTM, but noticed a couple of issues related to theming, not sure if they are known/expected at this stage:

In dark theme, there is a white(ish) border around the prompt:
Screenshot_20241105_150615

When switching theme while the prompt is visible, its content becomes blank:
Screenshot_20241105_150931
Screenshot_20241105_151033

broken-site/broken-site-api/src/main/java/com/duckduckgo/brokensite/api/BrokenSitePrompt.kt Outdated

suspend fun userAcceptedPrompt()

fun isFeatureEnabled(): Boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

The value of the toggle is stored in shared prefs, so this method is not safe to use from main thread - can we make it suspend fun + switch context to IO dispatcher for accessing the flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should be safe because we're accessing the cached value, not the shared prefs one

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true in case of subsequent invocations of this method - during the first call, cache is populated synchronously, so this method may block whichever thread is executing it. I'm not sure if you ever intend to execute this method from main thread - ideally, we shouldn't do any I/O operation on main thread if we can avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, you're right, this one doesn't come from DataStore!

...te/broken-site-impl/src/main/java/com/duckduckgo/brokensite/impl/BrokenSitePomptDataStore.kt
@@ -45,6 +50,10 @@ interface BrokenSitePomptDataStore {

suspend fun setCoolDownDays(days: Int)
fun getCoolDownDays(): Int
suspend fun setDismissStreak(streak: Int)
fun getDismissStreak(): Int
suspend fun setNextShownDate(nextShownDate: LocalDate?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm if I understand the logic correctly, if the prompt has coolDownDays = 1, that doesn't mean "don't show the prompt for 24h after it was dismissed", it means "don't show the prompt until the next calendar day after it was dismissed" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, it does, but a subsequent PR already migrates to the 24h approach

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-ff branch from f4943ba to 6a2eb12 Compare November 7, 2024 08:41
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch 5 times, most recently from 2162998 to fee4c7f Compare November 7, 2024 17:19
Base automatically changed from feature/cris/broken-site/add-ff to feature/cris/broken-site/base November 7, 2024 17:26
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/broken-site/add-toast-ui branch from fee4c7f to c91ebca Compare November 7, 2024 17:29
@CrisBarreiro CrisBarreiro merged commit c91ebca into feature/cris/broken-site/base Nov 7, 2024
4 of 5 checks passed
@CrisBarreiro CrisBarreiro deleted the feature/cris/broken-site/add-toast-ui branch November 7, 2024 17:30
@CrisBarreiro CrisBarreiro mentioned this pull request Nov 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmac012 lmac012 lmac012 approved these changes

@malmstein malmstein Awaiting requested review from malmstein

@nalcalag nalcalag Awaiting requested review from nalcalag

Assignees

@lmac012 lmac012

Labels
STACKED PR - DO NOT MERGE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@CrisBarreiro @lmac012