Skip to content
This repository was archived by the owner on Apr 4, 2022. It is now read-only.

notifications #992

Merged
merged 18 commits into from
Mar 22, 2022
Merged

notifications #992

merged 18 commits into from
Mar 22, 2022

Conversation

maria-vslvn
Copy link
Contributor

Summary

Closes #347

In order to reproduce this issue:

  1. Create an order
  2. While the order is in the 'Open' status, open the order details in the Explorer
  3. Switch off the internet connection

@github-actions
Copy link

@elena-zh
Copy link

Hi @maria-vslvn !
The notification itself looks good to me in a desktop view.

Several notes and questions:

  1. Notification text is different than is supposed to be in Failed to fetch notification #347 (comment) . Besides, there is no 'try again' link that should refresh the page.

  2. I was not able to see error messages on User details page. In the future, we will need to show it on Tx details as well.
    order list

  3. I was not able to see this warning (steps to reproduce are here Failed to fetch notification #347 (comment))
    image

  4. The warning is not adjusted to a mobile/tablet views
    mobile

  5. It would be nice to add margins between text lines in the notification
    magrings

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Noted above

@maria-vslvn
Copy link
Contributor Author

maria-vslvn commented Jan 18, 2022

@alfetopito @ramirotw i had to create two types of error display, but the object i got for errors was like {string: string}. And i just displayed the text. So the only way i thought to separate network error to not network one was with navigator.onLine. I'm pretty not sure about the way i separated the errors. So i think i need your help here, or any thoughts, because i'm mostly sure it's wrong:) Also @elena-zh said that the text is different from what was on the mockups, so should i display what comes from erorrs, or change it somehow before display?
@alongoni i added some icons as well, from fontawesome, but they were absent in the modules, so i added, i think we can have something like this for keeping custom icons?

@maria-vslvn
Copy link
Contributor Author

@henrypalacios could you please take a look at this issue? i'm having issues at the point of how should i work with the error new type, how could i get or calculate the type and so on

@ramirotw ramirotw self-requested a review January 31, 2022 15:45
@ramirotw
Copy link
Contributor

ramirotw commented Mar 16, 2022

@elena-zh please re-test.

  • User Details now has the notifications on top
  • I added a new ConnectionStatus component which listens to the browser's internet connectivity and displays itself whenever the connection is down. This will give the user more info as to why any request could have failed
  • I also shortened the order id in the notification error so it doesn't break on mobile

@ramirotw
Copy link
Contributor

@alfetopito the build fails with:

ERROR in Sentry CLI Plugin: Command failed: /home/runner/work/gp-ui/gp-ui/node_modules/@sentry/cli/sentry-cli releases new [email protected]
error: API request failed
  caused by: sentry reported an error: You do not have permission to perform this action. (http status: 403)

any idea why?

@ramirotw ramirotw removed their request for review March 16, 2022 20:31
@ramirotw ramirotw added the Protofire task to the protofire team label Mar 16, 2022
@alfetopito
Copy link
Contributor

@alfetopito the build fails with:

ERROR in Sentry CLI Plugin: Command failed: /home/runner/work/gp-ui/gp-ui/node_modules/@sentry/cli/sentry-cli releases new [email protected]
error: API request failed
  caused by: sentry reported an error: You do not have permission to perform this action. (http status: 403)

any idea why?

Yeah, I think so.
We are migrating to our new email accounts, and our old accounts have been removed from Sentry.
But, access tokens were bound to user accounts, so all existing tokens no longer work.

We'll need to update sentry auth tokens before it succeeds.

@W3stside or @anxolin are working on a fix.

@elena-zh elena-zh self-requested a review March 17, 2022 08:59
@elena-zh
Copy link

elena-zh commented Mar 21, 2022

@ramirotw , changes LGTM!
Notifications' texts are different from the ones in the mock-up, but they are clear and seems understandable to a user, from my perspective. But let wait for @alfetopito tests.

Tiny issue: the banner is blinking each time when the app is trying to update its state: https://watch.screencastify.com/v/87m3yXRRQNZUcNEWsCxx
Could we do anything with this?

Also, we have added a new page to the Explorer: Transactions. So it would be nice to see these banners there as well.
Let me know please if I need to create a separate issue for this.
Thanks

@ramirotw
Copy link
Contributor

@elena-zh both issues should be fixed

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Minor comments regarding the code.

Regarding the storybook, it's a bit odd. It shows up and quickly disappears

Screen.Recording.2022-03-21.at.15.48.49.mov

Not high priority though. If this can be fixed quickly go for it, otherwise I'm fine leaving as is.

@ramirotw
Copy link
Contributor

Regarding the storybook, it's a bit odd. It shows up and quickly disappears

@alfetopito fixed

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Changes LGTM now!

@ramirotw ramirotw merged commit 514b8f6 into develop Mar 22, 2022
@ramirotw ramirotw deleted the feature/347-notifications-failed2fetch branch March 22, 2022 13:09
This was referenced Apr 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Protofire task to the protofire team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to fetch notification
6 participants