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

Notifications frontend #904

Merged
merged 116 commits into from
Sep 4, 2019
Merged

Notifications frontend #904

merged 116 commits into from
Sep 4, 2019

Conversation

2color
Copy link
Contributor

@2color 2color commented Aug 7, 2019 •
edited
Loading

What

This builds on top of the changes in newstyle

  • Add subsection routing to preferences route to accomodate for multiple notifications routes
  • Custom hook to derive notification auth state from local storage
  • Functions for interacting with the API
  • Components for logging in, awaiting login, and rendering the subscriptions

Open UI/UX question

TODO

  • Add Logout button
  • Add delete account button
  • Add management link to event emails
  • Form validation - email address
  • Form validation - add icons
  • Subscriptions table - unsubscribe
  • Subscriptions table filtering
  • Get list of relevant events / filter some events
  • Filter out existing events from creation form
  • Return appName when getting subscriptions and render in table in app column
  • Fix bug with different apps having the same event name not showing in form if subscribed to one
  • Check adding the minime token contract as an "app" (not applicable as there are many challenges and differences from Aragon app contracts)
  • Cancel request if component is unmounted
  • Awaiting confirmation updated design
  • Verification failed updated design
  • subscribed to all updated design
  • Optimise notifications.svg.
  • TextInput Adornment focus out problem Notifications frontend #904 (comment)

TODO - Paty review

  • Order subscriptions by creation time
  • Global error page
  • Delete email/subscription confirmation modal
  • verification loading state match designs

@auto-assign auto-assign bot requested review from AquiGorka, bpierre and sohkai August 7, 2019 17:04
@vercel vercel bot temporarily deployed to staging August 7, 2019 17:06 Inactive
@vercel vercel bot temporarily deployed to staging August 8, 2019 14:27 Inactive
@vercel vercel bot temporarily deployed to staging August 9, 2019 10:11 Inactive
AquiGorka
AquiGorka reviewed Aug 9, 2019
View reviewed changes
src/components/GlobalPreferences/Notifications/NotificationsLogin.js Outdated
return (
<Box heading="Email notifications">
<NotificationImage />
{apiError && <Info mode="error">Error logging in:{apiError}</Info>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets just double check what should be the correct spacing here:
image
cc @dizzypaty
and we can help out the user with some other message, I was thrown off by "Failed to fetch".

@vercel vercel bot temporarily deployed to staging August 9, 2019 12:08 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 10:42 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 13:17 Inactive
@2color
Merge remote-tracking branch 'origin/newstyle' into newstyle-notifica…
2bda588 
…tions

* origin/newstyle:
  Optimize icons (#943)
  PropTypes: move fetch-required fields of AppType to be non-requi… (#946)
  Update ENS API changes from @aragon/wrapper (#944)
  Permissions: fix debounced search state on clearing filters (#942)
  Updates for aragonUI and other clean up (#939)
  AppCenter: update copy (#940)
@2color
Set consistent padding of 2/3 GU
a3205d1
@vercel vercel bot temporarily deployed to staging September 2, 2019 13:35 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 13:40 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 15:38 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 15:59 Inactive
@2color
Unsubscribe from table modal
8eebf90
@2color
Render form loading only while apps are loading
b8444bc 
Because every create triggers a fetch, avoid flicker while refetching
after creating a subscription
@vercel vercel bot temporarily deployed to staging September 2, 2019 16:35 Inactive
@vercel vercel bot temporarily deployed to staging September 2, 2019 16:50 Inactive
@bpierre bpierre changed the base branch from newstyle to master September 2, 2019 18:53
@vercel vercel bot temporarily deployed to staging September 3, 2019 10:23 Inactive
@vercel vercel bot temporarily deployed to staging September 3, 2019 16:46 Inactive
@vercel vercel bot temporarily deployed to staging September 3, 2019 16:53 Inactive
@2color 2color merged commit 5eada1c into master Sep 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the newstyle-notifications branch September 4, 2019 08:12
sohkai
sohkai reviewed Sep 8, 2019
View reviewed changes
src/components/GlobalPreferences/Notifications/Notifications.js
// network/service error
setAuthState(AUTH_SERVICE_UNAVAILABLE)
} else {
setAuthState(AUTH_AUTHENTICATION_FAILED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check for an UnauthorizedError or ExpiredTokenError?

Or perhaps we should have a custom error for the service being unavailable (ManageNotifcations is also checking for TypeError)?

TypeError seems pretty generic to know that it the service is unavailable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't we check for an UnauthorizedError or ExpiredTokenError?

That depends if we want to handle the two cases the same way. In both cases we currently require the user the log in again, hence the general catch.

TypeError seems pretty generic to know that it the service is unavailable.

TypeError is the standard error type for network errors. I didn't want to deviate too much from the fetch spec. https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch#Checking_that_the_fetch_was_successful

Copy link
Contributor

Choose a reason for hiding this comment

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

Did not know that TypeError was the standard error type for fetch()!

src/components/GlobalPreferences/Notifications/Notifications.js Show resolved Hide resolved
src/components/GlobalPreferences/Notifications/NotificationsInfoBox.js Show resolved Hide resolved
src/components/GlobalPreferences/Notifications/NotificationsLogin.js Show resolved Hide resolved
src/components/GlobalPreferences/Notifications/NotificationsVerify.js Show resolved Hide resolved
src/components/GlobalPreferences/Notifications/SubscriptionsTable.js
</div>
)}
</div>
<SubscriptionFilters
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have the same dropdown behaviour in the notification filters as elsewhere.

Each dropdown should have 'All' as its first item, and we should default the filter's state to -1 (unselected) if the first item is selected.

@luisivan luisivan mentioned this pull request Sep 8, 2019
Closed
95 tasks
@sohkai sohkai mentioned this pull request Sep 8, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dizzypaty dizzypaty dizzypaty left review comments

@sohkai sohkai sohkai left review comments

@AquiGorka AquiGorka AquiGorka left review comments

@bpierre bpierre Awaiting requested review from bpierre

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@2color @sohkai @bpierre @AquiGorka @dizzypaty