Notifications: contextualize email notifications #327
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). |
| if (!asyncCancelled.current) { | ||
| setEmail(email) | ||
| if (needsSignature !== Boolean(response.needsSignature)) { | ||
| setNeedsSignature(!needsSignature) |
There was a problem hiding this comment.
Trying to figure out how are you thinking to handle the action after the signature? i mean after the signature is successful we need to call again the subscribeToNotifications again.
And the other question that i have is : given that here we don't have an useEffect that listen to the needsSignature state, i guess that the plan is to have that useEffect in the manager?
There was a problem hiding this comment.
given that here we don't have an useEffect that listen to the needsSignature state, i guess that the plan is to have that useEffect in the manager?
Yes, exactly, that was something I wanted to leave to the state machine manager, given that we could have different machines based on the area (preferences or modal).
Trying to figure out how are you thinking to handle the action after the signature?
Similar to above, I was hoping the manager would handle this. These functions just attempt to "optimistically" call the API, and if it fails, it'll let us know either with the error or a state change.
| } | ||
|
|
||
| const getEmail = async () => { | ||
| const { needsSignature, email } = await getJurorEmail(account) |
There was a problem hiding this comment.
Giving a second thought to this, i think that here we are going to be fetching the juror email every time an account is connected, and when the cookie has expired we are going to ask the juror to sign a message just for connecting his account and not sure if that is what we want.
The way i see this is that we need to do that only when we are subscribing to notifications through the modal or entering the global preferences not every time the juror connect his account to interact with the dashboard.
What do you think?
There was a problem hiding this comment.
Good point. Yes, it sounds like we should do this in a callback instead.
| const fetchSubscriptionDetails = async () => { | ||
| const response = await getSubscriptionDetails(account) | ||
|
|
||
| if (!asyncCancelled.current) { |
There was a problem hiding this comment.
I used the same asyncCancelled ref here and after, but thinking about it, useEffects() should contain their own local cancellation.
| @@ -0,0 +1,142 @@ | |||
| import React, { | |||
There was a problem hiding this comment.
Nitpick: we may want to rename this file to be EmailNotificationsApiProvider for clarity.
| const response = await subscribeToNotifications(account, email) | ||
|
|
||
| if (response.error && !response.needsSignature) { | ||
| return response.error |
There was a problem hiding this comment.
Thinking about this, we may want to throw the error instead (and perhaps all the requests should be throwing those errors, similar to https://github.com/aragon/aragon/blob/master/src/components/GlobalPreferences/Notifications/notification-service-api.js#L60).
sohkai
force-pushed
the
email-notifications-context
branch
from
9aaf1a6
to
2c2dbee
Compare
Stub/spike of moving the email notifications information up to a context.