Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

feat: Notification system update #73

Merged
merged 36 commits into from
Sep 22, 2021
Merged

feat: Notification system update #73

merged 36 commits into from
Sep 22, 2021

Conversation

KaWaite
Copy link
Member

@KaWaite KaWaite commented Sep 6, 2021

Overview

This is an update to the notification system. We were creating notifications in a couple different ways and had duplicate code, so this cleans things up and is trying to make things simpler.

What I've done

  • Moved notification code into its own hooks file and created a notification wrapper component that sits above the router so it doesn't refresh on page changes.
  • Added more notifications + updated translations

What I haven't done

  • GQL errors coming from the backend aren't often user-facing appropriate. Backend error handling still needs to be sorted out eventually

How I tested

  • Did my best to test all notifications. Some errors I couldn't test for one reason or another.

Screenshot

Screen Shot 2021-09-06 at 16 26 27

Screen Shot 2021-09-06 at 16 26 40

Screen Shot 2021-09-06 at 16 27 00

Which point I want you to review particularly

  • Translations (@rot1024 @HideBa)
  • Where the notification code should go (I wasn't sure the best place, but since it wraps the whole app similar to the various providers, I thought maybe it should be where I put it in the src directory)

test cases

Dashboard

  • Create project
  • Create workspace
    Settings
  • change password
  • create project
  • archive project
  • delete project
  • create workspace
  • delete workspace
    Earth Editor
  • Publish project
  • Unpublish project
  • Add dataset

KaWaite added 8 commits August 30, 2021 15:37
Move notification code into globalHooks.
Remove duplicate code.
Update types.
Add transition to notifBanner.
Update project creation notifications.
Clean up code.
WIP: Add missing notifications.
Move useEffects into hooks.
Use notifications for errors directly(when possible).
Update some text.
Fix gql error handling.
Fix where gql errors were overwritten by other notifications.
Add notifications for dataset import.
Add better notif handling for archiving projects.
Add notif for plugin installation.
Add notif for adding assets.
@netlify
Copy link

netlify bot commented Sep 6, 2021

✔️ Deploy Preview for reearth-web ready!

🔨 Explore the source changes: 8b527c7

🔍 Inspect the deploy log: https://app.netlify.com/sites/reearth-web/deploys/614a9d08209f010007291a59

😎 Browse the preview: https://deploy-preview-73--reearth-web.netlify.app/

@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #73 (8b527c7) into main (8e78f94) will increase coverage by 0.20%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   57.46%   57.67%   +0.20%     
==========================================
  Files          44       46       +2     
  Lines         703      730      +27     
  Branches      104      114      +10     
==========================================
+ Hits          404      421      +17     
- Misses        257      261       +4     
- Partials       42       48       +6     
Impacted Files Coverage Δ
src/gql/provider.tsx 5.26% <0.00%> (-0.30%) ⬇️
src/state/index.ts 75.86% <ø> (ø)
src/theme/colors.ts 100.00% <ø> (ø)
src/components/atoms/Notification/index.tsx 66.66% <66.66%> (ø)
src/components/atoms/Text/index.tsx 63.63% <0.00%> (ø)

@KaWaite KaWaite marked this pull request as ready for review September 6, 2021 07:21
Copy link
Member

@rot1024 rot1024 left a comment

Choose a reason for hiding this comment

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

Let's try to write a component unit test!

src/notifications/README.md Outdated Show resolved Hide resolved
src/notifications/hooks.ts Outdated Show resolved Hide resolved
src/notifications/hooks.ts Outdated Show resolved Hide resolved
src/notifications/index.tsx Outdated Show resolved Hide resolved
src/app.tsx Outdated Show resolved Hide resolved
src/app.tsx Outdated Show resolved Hide resolved
KaWaite added 2 commits September 6, 2021 16:53
Update all imports.
Remove notifications README.
Copy link
Member

@rot1024 rot1024 left a comment

Choose a reason for hiding this comment

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

Would you like to write unit tests for Notification component?

src/components/pages/TopPage/hooks.ts Outdated Show resolved Hide resolved
src/notifications/hooks.ts Outdated Show resolved Hide resolved
Copy link
Member

@rot1024 rot1024 left a comment

Choose a reason for hiding this comment

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

Molecules should not be responsible for notifications; please run notifications in organisms. Otherwise, the dependencies between components will become too complex.

rot1024
rot1024 previously approved these changes Sep 15, 2021
Copy link
Member

@HideBa HideBa left a comment

Choose a reason for hiding this comment

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

I quickly reviewed and left a comment.

src/components/molecules/Common/Notification/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@HideBa HideBa left a comment

Choose a reason for hiding this comment

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

LGTM!

@KaWaite KaWaite merged commit 92cdbb4 into main Sep 22, 2021
@KaWaite KaWaite deleted the feat-notification-sys branch September 22, 2021 04:55
keiya01 pushed a commit that referenced this pull request Apr 25, 2023
* feat: cluster domain models (#69)

* Co-authored-by: mimoham24 <mimoham24@users.noreply.github.com>

* feat: domain models

* add cluster extenstion type

* add cluster extension type test case

* fix cluster layers

* resolve notes

* update manifest

* fix manifest.yml margin

* refactor models

* resolve notes

* fix manifest file

* resolve notes

* resolve notes

Co-authored-by: HideBa <baba.papa1120.ba@gmail.com>

* feat: cluster datalayer (#73)

* feat: cluster graphql/mongo (#74)

* feat: cluster datalayer

* feat: cluster CRUD

* resolve notes

* resolve notes

* add test cases

Co-authored-by: HideBa <baba.papa1120.ba@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants