-
Notifications
You must be signed in to change notification settings - Fork 393
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.
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
Monitoring XP drop #3679
Monitoring XP drop #3679
Conversation
7c9e96b
to
55d57ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just small adjustments 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few small notes. Will revisit tomorrow to decide what if anything is blocking before merging + cutting a release.
background/services/island/index.ts
Outdated
const options = { | ||
title: "Weekly XP distributed", | ||
message: "Visit Subscape to see if you are eligible", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does clicking on this notification have any impact? This was the goal of the callback
part of the notification method, which would allow for a callback that opens Subscape (via browser.tabs.create
, same as the Subscape logo link I believe).
…browser object change
56030ad
to
eede991
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broad note: across the board, if we're using promises we should use async
/await
, not mix it in with .then
.
I didn't get a chance to give this a full test drive with the switch back to browser
WebExtension APIs. I think if we can confirm that permissions are prompted for properly (remember you have to uninstall/reinstall the extension for Chrome to reprompt for permission, possibly even change the extension id by installing it from a different location), the rest of the notes are non-blocking and we should land + cut the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
61530a3
to
f76595b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise we're looking solid here. Will test drive shortly.
import { HOUR } from "../../constants" | ||
|
||
const TAHO_ICON_URL = | ||
"https://taho.xyz/icons/icon-144x144.png?v=41306c4d4e6795cdeaecc31bd794f68e" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocking, but we should have an extension-local URL available for this I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we can do it, also for the wallet-connection-handler (the same approach).
Confirmed the notification permission is requested correctly, notification goes out, and toggle works as expected. |
Notably: I did not test attachment to the event, as that's not really doable without a lot of extra legwork. Code seems like it should do the thing, we'll find out in the wild 🤷 |
## What's Changed * v0.52.0 by @jagodarybacka in #3678 * Update release test list by @michalinacienciala in #3682 * Monitoring XP drop by @ioay in #3679 ## New Contributors * @ioay made their first contribution in #3679 **Full Changelog**: v0.52.0...v0.53.0 Latest build: [extension-builds-3690](https://github.com/tahowallet/extension/suites/18828582723/artifacts/1098086930) (as of Thu, 07 Dec 2023 03:26:04 GMT).
What
Showing notifications at the time of XP drop.
Prerequisite
The user allows the notification to be displayed.
Testing
Result
Important!
Do not merge! I'll do that, before merge we need to change notifiaction time threshold from 30sec(for testing purposes) to 24h:
NOTIFICATIONS_XP_DROP_THRESHOLD_MS_FOR_TESTING_PURPOSE
toNOTIFICATIONS_XP_DROP_THRESHOLD_MS
Latest build: extension-builds-3679 (as of Wed, 06 Dec 2023 22:39:43 GMT).