-
Notifications
You must be signed in to change notification settings - Fork 0
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
Notifications #44
Notifications #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my brief time with it, it is working quite well! Good job.
I do have some suggestions for changes:
- I think we should skip the
dispatch(setLoading)
. Given that the notifications seems to load quite quickly, I don't think it is really necessary. Plus, the quick flash of the darkened loading background is jarring and almost makes it seem like something went wrong. - The notifications themselves seem a bit cluttered with all the icons. I would suggest putting the Mark as Read and the Delete options into a separate menu, maybe like this. Alternatively, remove the option to individually mark as read, and just have an x icon for delete like this
- set a fixed width for the notification menu, text overflow for the message content and change the date-time to a more common format (e.g. "Jul 14 4:15 pm")
- I would imagine that, once we have all of it hooked up, we would have more descriptive notification messages such as "tradefore436 has made an offer on Fresh Tomatoes" especially since we may not be able to have clickable links to the offer itself. Would we want to add the username and posting title to the notifications separately to be able to highlight them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really impressed that you got this up so quickly! Thanks for your hard work :D.
Some fixes to be made for styling (mostly the spacing on the navBar and the workaround for the material-ui bug. Unofficially in slack, Ness told me that she prefers your positioning of the dropdown menu, so you can disregard my comment on that. Once she officially comments on it, I'll mark that thread as resolved xD).
With regards to Ness' comments:
In my brief time with it, it is working quite well! Good job.
I do have some suggestions for changes:
- I think we should skip the
dispatch(setLoading)
. Given that the notifications seems to load quite quickly, I don't think it is really necessary. Plus, the quick flash of the darkened loading background is jarring and almost makes it seem like something went wrong.- The notifications themselves seem a bit cluttered with all the icons. I would suggest putting the Mark as Read and the Delete options into a separate menu, maybe like this. Alternatively, remove the option to individually mark as read, and just have an x icon for delete like this
- set a fixed width for the notification menu, text overflow for the message content and change the date-time to a more common format (e.g. "Jul 14 4:15 pm")
- I would imagine that, once we have all of it hooked up, we would have more descriptive notification messages such as "tradeforce436 has made an offer on Fresh Tomatoes" especially since we may not be able to have clickable links to the offer itself. Would we want to add the username and posting title to the notifications separately to be able to highlight them?
-
I agree that the loading is a bit jarring. I would also prefer that it wasn't there.
-
I also agree about the icons. In addition to her comments, I would also suggest that the radioIcon be swapped out for the check icon (see reference to facebook) if we do decide to keep the individual mark as read functionality. The radioIcon makes me think that I'm selecting this notification in preparation for a future action (such as delete all) rather than for marking it as unread
-
As you can probably see in some of my screenshots, a longer text overflows into the icons. We'll probably need to wrap the message or truncate it after a certain length. (Agree with Ness on the date format)
-
Hmm. That's a good point. Are you thinking the userName of the offerer or the one who is receiving the notification? Would it be better to ask for the offering in question back as a response so we have access to all that info? Thinking on it now, Though it would be difficult to redirect the notified user to the offering in question on the userProfile page, I could (at least for new offers) display a modal with the contents of the new offer? What do you both think? @jenessatan @abid-salahi
Thanks again Abid! It's working well!
So notifications are pretty much functional now once the notifications backend is also merged. The thing that remains is to integrate it with the actual making of offerings, as well as accepting/rejecting of offers. Also need to add the notifications count to the icon.
Right now i have made it so we support 3 types of notifications:
For testing purposes to see it in action, use Postman and make an api call as follow (assuming you have the notifications backend fetched and running)
here is a sneak peak of what notifications look like. Feel free to make suggestions as to how to make them look better or any changes needed!