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.

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

Improve (Error) Notifications #89

Closed
vmatsiiako opened this issue Dec 7, 2022 · 14 comments · Fixed by #92
Closed

Improve (Error) Notifications #89

vmatsiiako opened this issue Dec 7, 2022 · 14 comments · Fixed by #92
Labels

Comments

@vmatsiiako
Copy link
Collaborator

Feature description

It would be really great if notification auto-disappeared (e.g., a user had a notification saying that they have some duplicate keys and then it would disappear like int he attached example) so that they don't need to remove those themselves

The following is an example of how PostHog does. Ours doesn't need to be so well-animated, but I think this would an end goal :)
https://user-images.githubusercontent.com/78047717/206074042-f1b9c508-0993-4cc0-a33a-d7f0e3b3a1a2.mov

I also think that we generally have to add more notification (when secrets are saved, we should tell people something like "Successfully saved the secrets"). The auto-disappearence would be amazing in this case.

Why would it be useful?

It would a huge improvement for UX. I think many people would benefit from it.

@vmatsiiako vmatsiiako added 🚀 feature request New feature or request help wanted Open for contributions from the community labels Dec 7, 2022
@vmatsiiako
Copy link
Collaborator Author

I'm attaching the video to the comment because it doesn't autoplay above for some reason.

Screen.Recording.2022-12-06.at.19.07.31.mov

@vmatsiiako
Copy link
Collaborator Author

@asharonbaltazar This is something that I had in mind. Also, what do you think if we moved notifications to bottom right of the screen?

@asharonbaltazar
Copy link
Contributor

Sounds about what I had in mind. Also, ack regarding the bottom-right positioning

@asharonbaltazar
Copy link
Contributor

You can assign it to me. I don't think it'll take long to push something

@asharonbaltazar
Copy link
Contributor

Can you please list all the instances where you want notifications created?

@vmatsiiako
Copy link
Collaborator Author

vmatsiiako commented Dec 7, 2022

Yes! I think these ones are the most important ones:

  • Successfully saved the keys (savePush in dashboard/[id].js)
  • Error: can't save an environment variable with an empty value (savePush in dashboard/[id].js)
  • Organization invite resent successfully (submitAddUserModal in setting/org/[id].js)
  • User was added to an organization successfully (submitAddUserModal in setting/org/[id].js) - I just realized we can't differentiate between these two ^
  • Project created (submitModal in Layout.tsx)
  • User was added to project successfully (submitAddModal in users/[id].js)

Feel free to implement anything else you think is useful!

@vmatsiiako vmatsiiako added ✨ UX and removed 🚀 feature request New feature or request help wanted Open for contributions from the community labels Dec 7, 2022
@asharonbaltazar
Copy link
Contributor

@mv-turtle Would you mind linking the respective functions mentioned in your comment above? I'd really appreciate that!

@vmatsiiako
Copy link
Collaborator Author

@mv-turtle Would you mind linking the respective functions mentioned in your comment above? I'd really appreciate that!

Of course! I just updated the comment. Sorry for the wait

@asharonbaltazar
Copy link
Contributor

Don't sweat it. I'll make another PR for these changes.

@asharonbaltazar
Copy link
Contributor

@mv-turtle After looking at the code, I decided that, for now, it's probably best to avoid adding notifications until two things have been taken care of:

  1. We can't append functions for successful async functions. Having a Created Project Successfully! notification on failure is confusing... Currently, there's no way to differentiate other than a console.log
  2. router.reload() is also gonna get in the way. router.reload() is called after each network call, successful or not. Refreshing the page reloads the DOM, killing all notification processes and rendering createNotification useless.

I believe both issues one and two can be solved by using a library such as React-Query.

@vmatsiiako
Copy link
Collaborator Author

@asharonbaltazar Right, I think this makes sense! Indeed, router.reload() should ideally be fully substituted to something more efficient. This was more of a hack from my side for quicker development.

React-Query looks great - seems like a very popular package. I can take a look at it soon-ish. Would you be interested in solving those 2 issues? Thank you for your help :)

@asharonbaltazar
Copy link
Contributor

asharonbaltazar commented Dec 8, 2022

@mv-turtle While I appreciate the offer, this is a large feature that'll require many hours of work and collaboration, and I simply can't dedicate that much time. I'm not as invested in the project as Infisical members.

I still have the .yml feature to start , and I'm happy to help out with smaller, more localized features. Hope you understand!

@vmatsiiako
Copy link
Collaborator Author

Yes, don’t worry at all about this! Thank you so much :)

@asharonbaltazar asharonbaltazar removed their assignment Aug 18, 2023
@akhilmhdh
Copy link
Member

Closing as stale and we have now a good error notification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants