show follow notification in tab #1727

pelumy · 2025-01-08T21:22:47Z

Issues covered

127
P/S: This needs the backend to send follow notifications as a silent one and not an alert. This needs to be taken into consideration before merging.

Description

I wasn't expecting this PR to turn out this huge, but it did 😿. It contains the following changes:

Creates a new core data model version. The NosNotification entity needed a relationship with the Event entity.
Refactors the NosNotificationViewModel and NotificationView.
Creates a NosNotification and local notification for a silent follow notification.
Redefines the NotificationView to shows 3 tabs: "Follows", "In Network" and "Out of Network".
Deleting NosNotification reference when an Event is deleted in the DatabaseCleaner. Thanks for fixing this @mplorentz.
Deleting NosNotification and Event entities on launch based on a UserDefaults flag.

How to test

Build the app.
Select the Notifications option in the bottom tabs.
- You should now see three tabs "Follows", "In Network" and "Out of Network".
Please clone this to send a silent push notification. You can change the follower's npub in the json, this example uses Josh's pub. Don't forget to copy your device token and update that part in the notification console. This is a notion doc on how to send push notifications from the console.
- You should see the notification appear in the Follows tab and also in your iPhone's home Screen.

Screenshots/Video

Post screenshots or video showing your changes, ideally showing how the app worked before and after these changes. Delete this section if this PR contains no visual changes.

Notification tab view

Before	After

Before	After

Video showing follows notification, inNetwork and outOfNetwork.
https://github.com/user-attachments/assets/a08cab0c-0a66-4099-9b8c-3b31b3bee6eb
N/B: I updated the UI after making the video. The screenshot above is the correct UI.

…ation

joshuatbrown · 2025-01-15T18:08:28Z

👀

mplorentz

Hey Itunu, the code looks good to me! There are a couple minor issues I noticed, and some of them are design related. I'm ok to merge and ship this as-is, and we can address these in future PRs.

The first is that I think it would look a lot better to have "Follows" and "Out of Network" centered between "In Network", rather than having them pushed to the edge. This is only an issue on mac/ipad. Also I think it would be good if their tap targets covered the full height of the bar, instead of just the height of the text.

This is totally a design thing, but I wish the default tab was "in network", not "Follows". Personally I think I'll be looking at "In Network" the most, "Out of Network" the second-most, and "Follows" the least. What do you think? We could ask about making a change if you also care.
I don't like that the badge count is cleared as soon as I tap the notifications tab, even if I haven't viewed all the notifications. We could only mark the visible notifications as read. Again, not a blocker.

mplorentz · 2025-01-23T18:39:18Z

Nos/Models/CoreData/Author+CoreDataClass.swift

@@ -406,4 +406,11 @@ import Logger
        // Publish the modified list
        await currentUser.publishMuteList(keys: Array(Set(mutedList)))
    }
+
+    /// Returns true if this event tagged the given author.
+    func this(author: Author) -> Bool {


I think this name should be a little more descriptive. If I just saw it at the call site like authorA.this(author: authorB) I would probably guess that it was some sort of equality check. The comment is confusing me too, because it says "if this event tagged" but this is a function on Author, so what event are we talking about?

mplorentz · 2025-01-23T18:39:42Z

Nos/Models/CoreData/Event+Fetching.swift

@@ -96,7 +97,90 @@ extension Event {

        return fetchRequest
    }
-
+
+    /// A request for all out-Of-Network events that the given user should receive.


Suggested change

/// A request for all out-Of-Network events that the given user should receive.

/// A request for all out-Of-Network events that the given user should be shown a notification for.

mplorentz

I meant to click approve on the last one 🙃

pelumy · 2025-01-29T12:18:25Z

Hey Itunu, the code looks good to me! There are a couple minor issues I noticed, and some of them are design related. I'm ok to merge and ship this as-is, and we can address these in future PRs.

The first is that I think it would look a lot better to have "Follows" and "Out of Network" centered between "In Network", rather than having them pushed to the edge. This is only an issue on mac/ipad. Also I think it would be good if their tap targets covered the full height of the bar, instead of just the height of the text.

This is totally a design thing, but I wish the default tab was "in network", not "Follows". Personally I think I'll be looking at "In Network" the most, "Out of Network" the second-most, and "Follows" the least. What do you think? We could ask about making a change if you also care.

I don't like that the badge count is cleared as soon as I tap the notifications tab, even if I haven't viewed all the notifications. We could only mark the visible notifications as read. Again, not a blocker.

Thanks for your feedbacks @mplorentz. The first point is a bother for me too, thanks for pointing it out. @Chardot what do you think?

For the UI change, I can throw a quick jab at it and see if the change can go with this PR.

…ation

Chardot · 2025-01-29T17:00:33Z

Hey Itunu, the code looks good to me! There are a couple minor issues I noticed, and some of them are design related. I'm ok to merge and ship this as-is, and we can address these in future PRs.
The first is that I think it would look a lot better to have "Follows" and "Out of Network" centered between "In Network", rather than having them pushed to the edge. This is only an issue on mac/ipad. Also I think it would be good if their tap targets covered the full height of the bar, instead of just the height of the text.

This is totally a design thing, but I wish the default tab was "in network", not "Follows". Personally I think I'll be looking at "In Network" the most, "Out of Network" the second-most, and "Follows" the least. What do you think? We could ask about making a change if you also care.

I don't like that the badge count is cleared as soon as I tap the notifications tab, even if I haven't viewed all the notifications. We could only mark the visible notifications as read. Again, not a blocker.

Thanks for your feedbacks @mplorentz. The first point is a bother for me too, thanks for pointing it out. @Chardot what do you think?

For the UI change, I can throw a quick jab at it and see if the change can go with this PR.

I agree with @mplorentz's observations about In Network being the default tab on this screen (in fact, that's how I designed it in the first place, but it got changed to Follow).

As discussed with @pelumy today, we decided to make In Network the default tab, while keeping its position in the center to comply with the design that Linda referenced in the ticket, and we can modify the tab bar in a future iteration.

Also agree that the tap surface should cover the full height of the tab bar.

One thing we discussed with @pelumy is how tabs should be spaced between and also centered in the tab bar so that in larger windows on iPads and desktop, the tabs don't get too separated across the screen. We decided to wrap the tabs with a centered container that keeps a maximum width just like the one @martindsq added in the profile view and Feed so that the note cards don't stretch to the full width of the window.

To illustrate how tabs are spaced between, here's an image I made in Figma:

Tab labels have variable width, but the space left in the tab bar is evenly split around them.

pelumy · 2025-01-29T18:36:00Z

Based on the above comments, this is the updated UI.

Mac/iPad:

iPhone:

mplorentz · 2025-01-29T20:22:31Z

@pelumy the Mac/iPad screenshot you posted doesn't seem to follow the "space around" spacing that Sebastian described. Is that something you were intending to include in this PR or to add somewhere else?

pelumy · 2025-01-29T22:38:59Z

@pelumy the Mac/iPad screenshot you posted doesn't seem to follow the "space around" spacing that Sebastian described. Is that something you were intending to include in this PR or to add somewhere else?

@mplorentz , it has been corrected. thanks for pointing it out:

…ation

mplorentz · 2025-01-31T19:29:14Z

👀

mplorentz

Unfortunately I'm still seeing one minor layout issue. The tab bar is not extending to the edges of the screen on my Mac. I think it is probably inside a container using .readabilityPadding(), but that should only be used on the content.

found a bug

…tary-social/nos into it/127-follow-notification

pelumy · 2025-01-31T20:15:26Z

Thanks for catching that @mplorentz. The UI has been updated.

…ation

pelumy · 2025-01-31T20:58:49Z

This new failing test is not failing for me locally. I'm not sure what is wrong.

mplorentz

I ran the tests again and they passed. They also pass for me locally. Let's chalk it up to a random failure. Merge away!

pelumy added 5 commits January 8, 2025 22:06

update appDelegate

e75f954

update NotificationViewModel

3ecfbb7

update coredata classes

09b5895

update push notification service

a05bdb7

update NotificationView

c49006f

pelumy requested review from mplorentz, joshuatbrown and bryanmontz as code owners January 8, 2025 21:22

pelumy marked this pull request as draft January 8, 2025 21:22

pelumy changed the title ~~Ishow follow notification in tab~~ show follow notification in tab Jan 8, 2025

pelumy added 17 commits January 9, 2025 19:21

clean up

dd9e6b4

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

5175b64

…ation

add changelog

9846f80

fix swiftlint warnings

0361260

fix swiftlint warning

98b792f

create new coredata model version

b66dcab

write notifications unit test

22fe772

update unit test

48e1a77

update notificationViewModel

33ed191

create Follows notification card

980c234

update Notification view and coredataclass

9619a7b

update databasecleaner

761a415

cleanup

f4f51cf

use localised strings

b23d1d9

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

c3d1074

…ation

fix comment

27de0ce

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

1f18b23

…ation

pelumy marked this pull request as ready for review January 13, 2025 21:24

fix UI and autoscroll

2fa4555

mplorentz reviewed Jan 28, 2025

View reviewed changes

mplorentz self-requested a review January 28, 2025 22:01

mplorentz previously approved these changes Jan 28, 2025

View reviewed changes

pelumy added 3 commits January 29, 2025 14:01

update function name and documentation

2b7c6b6

update UI

3be7add

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

75fad2d

…ation

update UI

ddd9b48

space around

1ed8ec7

pelumy and others added 2 commits January 29, 2025 23:39

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

d0b5f06

…ation

Merge branch 'main' into it/127-follow-notification

c1c2bc5

mplorentz requested a review from martindsq as a code owner January 31, 2025 19:29

mplorentz reviewed Jan 31, 2025

View reviewed changes

pelumy added 2 commits January 31, 2025 21:10

update notifications UI

b71e82c

Merge branch 'it/127-follow-notification' of https://github.com/plane…

0bd892b

…tary-social/nos into it/127-follow-notification

pelumy added 2 commits January 31, 2025 21:16

Merge remote-tracking branch 'origin/main' into it/127-follow-notific…

228eae6

…ation

fix failing test

7180481

mplorentz approved these changes Jan 31, 2025

View reviewed changes

pelumy added this pull request to the merge queue Jan 31, 2025

Merged via the queue into main with commit 5d0ca91 Jan 31, 2025
4 checks passed

pelumy deleted the it/127-follow-notification branch January 31, 2025 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

show follow notification in tab #1727

show follow notification in tab #1727

pelumy commented Jan 8, 2025 •

edited

Loading

joshuatbrown commented Jan 15, 2025

mplorentz left a comment

mplorentz Jan 23, 2025

mplorentz Jan 23, 2025

mplorentz left a comment

pelumy commented Jan 29, 2025

Chardot commented Jan 29, 2025

pelumy commented Jan 29, 2025

mplorentz commented Jan 29, 2025

pelumy commented Jan 29, 2025

mplorentz commented Jan 31, 2025

mplorentz left a comment

pelumy commented Jan 31, 2025

pelumy commented Jan 31, 2025

mplorentz left a comment

	/// A request for all out-Of-Network events that the given user should receive.
	/// A request for all out-Of-Network events that the given user should be shown a notification for.

show follow notification in tab #1727

show follow notification in tab #1727

Conversation

pelumy commented Jan 8, 2025 • edited Loading

Issues covered

Description

How to test

Screenshots/Video

joshuatbrown commented Jan 15, 2025

mplorentz left a comment

Choose a reason for hiding this comment

mplorentz Jan 23, 2025

Choose a reason for hiding this comment

mplorentz Jan 23, 2025

Choose a reason for hiding this comment

mplorentz left a comment

Choose a reason for hiding this comment

pelumy commented Jan 29, 2025

Chardot commented Jan 29, 2025

pelumy commented Jan 29, 2025

mplorentz commented Jan 29, 2025

pelumy commented Jan 29, 2025

mplorentz commented Jan 31, 2025

mplorentz left a comment

Choose a reason for hiding this comment

pelumy commented Jan 31, 2025

pelumy commented Jan 31, 2025

mplorentz left a comment

Choose a reason for hiding this comment

pelumy commented Jan 8, 2025 •

edited

Loading