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

Priority notifications #4798

Merged
merged 18 commits into from
Jul 24, 2024
Merged

Priority notifications #4798

merged 18 commits into from
Jul 24, 2024

Conversation

mozzius
Copy link
Member

@mozzius mozzius commented Jul 17, 2024

Adds a new page, /notifications/settings with a toggle to turn priority notifications on. Backend is not yet deployed at time of writing It's ready to go!

Note on behaviour - we get what the current priority setting is from listNotifications. If we call this immediately after, I was running into read-after-write issues, so it attempts it a few times before giving feedback to the user on whether it has succeeded or not. It disables the input while waiting.

It shows a spinner while waiting for the initial value, if it's not already cached (which it should be 9/10 times, since it reuses the query from the main page and you're likely coming from that page)

Screenshot 2024-07-19 at 15 02 07

Screenshot 2024-07-24 at 16 08 58

Copy link

render bot commented Jul 17, 2024

Copy link

github-actions bot commented Jul 17, 2024

Old size New size Diff
6.62 MB 6.63 MB 5.58 KB (0.08%)

@mozzius mozzius marked this pull request as ready for review July 19, 2024 22:04
@mozzius mozzius force-pushed the samuel/priority-notifications branch 2 times, most recently from 65e5adc to dd7449b Compare July 23, 2024 21:50
@mozzius mozzius force-pushed the samuel/priority-notifications branch from dd7449b to 7451a5a Compare July 23, 2024 23:59
@mozzius mozzius requested a review from gaearon July 24, 2024 15:12
@haileyok
Copy link
Contributor

Left two notes, but with a minimal test this works well.

I'm still concerned about one thing especially after using this but unrelated to the PR, I'll flag it in Slack.

@mozzius mozzius requested a review from haileyok July 24, 2024 17:22
mozzius and others added 3 commits July 24, 2024 18:40
Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

Tested a bunch of different states

  • Intermitent network
  • Going back before preference updates
  • Slow network and spam change

Things seem really good here. Weighed pros/cons of that mutation, and for now this seems okay, since it's already a little bizarre how we're getting the preference. I think we will want to start returning a preference object whenever we add more preferences here (like granular filters).

Gonna have to build for TF to test since OTA is borked. Great work @mozzius!!! 🥳

@mozzius mozzius merged commit cfb8a31 into main Jul 24, 2024
6 checks passed
@mozzius mozzius deleted the samuel/priority-notifications branch July 24, 2024 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants