Notif overhaul - Githubissues

NEUDitao commented 5 years ago

Hey y'all, been working on a notifications system overhaul a la https://github.com/ryanhugh/searchneu/issues/10. It's pretty functional (aside from a few memory leaks), but it's also really ugly. Dropping this here so people can start commenting on it!

NEUDitao commented 5 years ago

Tests are done. So the big issues still around

[ ] The await get(100) thing is just super sus. It's also causing a yarn fix fail. Tackling the slow download could be a thing, but that might be a while. We'll see though
[x] That one pesky memory leak that's still around when you log as new user. (memory leak doesn't seem to happen on mobile???) (as we mentioned, very difficult for me to test on mobile)
[x] Mobile

NEUDitao commented 5 years ago

Refactored the work flow as requested through Dms, again, cannot test on mobile because cOmPuTeR is dumbo

Only thing left is to remove the linting thing about the await in the for loop (since there's no other way to get the data there), or remove the for loop altogether through slow polling

NEUDitao commented 5 years ago

As discussed w/ Ryan, having a weird issue w/ FireFox on my end. Can anyone else test is signing up for notifications is currently broken on FF? (window.FB is not loading).

Also cannot test mobile properly on Chrome, though in theory it works. Working on that on my end currently.

edward-shen commented 5 years ago

Firefox has stronger encapsulation and stronger privacy defaults, including blocking trackers. It's possible that it's seeing FB on a non-FB site and blocking that.

Users might also face issues when they install the Mozilla-recommended Facebook Container, which encapsulates Facebook from the browser.

NEUDitao commented 5 years ago

Interesting, what would be your suggestion for this then? According to Stack Overflow, we might be shit outta luck.

Just to confirm, signing up for notifications no longer works on Prod either, just throws a white screen atcha.

I think the best option at this point would be to put up a warning if the user tries to sign up for notifications using Firefox, same as adblock.

ryanhugh commented 5 years ago

The live site works in my firefox. Lets look into this a bit more before throwing up a warning for ff - I think we can get it working :) I'll PM ya!

ryanhugh commented 5 years ago

Looks like the site doesn't work if FF is in strict Content Blocking mode - perhaps in a future PR we could throw up a warning if this is enabled

ryanhugh commented 5 years ago

Status of stuff:

I pushed a fix for the firefox thing in a different PR #85
I also made a bunch of changes to the sender id stuff #85 (lets keep the functionality in #85)
Removed /getUserData API and a bunch of code in user.js, which we will need for this PR and we can add it back again.

Todo here:

remove the firefox stuff
Cleanup the callback stuff
and the retry 100 stuff (will need to add back and change the /getUserData code i just removed)
some other cleanup
mobile support

Lmk if you wanna do this and I'll give some pointers!

NEUDitao commented 5 years ago

Some updates, since I haven't had a while.

This feature's taken a while! That's because from May to August I didn't have a computer, or internet consistently! Picking up work where it dropped off was difficult.
The feature works now. If you want to test it (please test it), just checkout the branch, make sure elasticsearch is running, and run it locally.
no u.
There still isn't a version of long polling done yet. That could probably be done in the future, but getting the feature out might be more valuable.

NEUDitao commented 5 years ago

Pulled a funny prank. Implemented long polling.

ryanhugh commented 5 years ago

Fix up a bunch of stuff on travis when you have a second! yarn fix

ryanhugh commented 5 years ago

Great work Eddy!!

@dajinchu Take a look when ya have a chance

Here's some comments

Can you expand this message to clarify what happened and tell the user what they are signed up for? Like, when I first click the send to messenger button am I signed up for notifications on the class or not - eg will I be notified if sections are added and removed from the class?

https://github.com/ryanhugh/searchneu/blob/master/backend/updater.js#L218

Also should we send a FB notification when they click the switch to tell them they are signed up for a new section?

Can you make the (i) buttons smaller/a lighter color to de-emphasize it compared to the other ui stuff?

When you first refresh the page, should we make it show the switches for classes people are signed up for? (instead of saying click here to sign up for notifications when they already did that and we know they did)

When there isn’t any sections, lets not say toggle the sections you want to be signed up for.

Make sure to go over all the edge cases and think about the user experience when using the feature in different scenarios - like what information are we telling the user when they take certain actions? :)

good work!

dajinchu commented 5 years ago

I agree with Ryan on the usability. Looks good on the elasticsearch end of things!

NEUDitao commented 4 years ago

God's in his heaven, and all is right in the world.

ryanhugh / searchneu

Notif overhaul #81