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

Refactor Inbox Page #1463

Merged
merged 14 commits into from
Jul 5, 2024
Merged

Refactor Inbox Page #1463

merged 14 commits into from
Jul 5, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Jun 21, 2024

Pull Request Description

This is a draft PR which refactors the inbox page to be more consistent with the rest of our UI. In particular:

  • Switch to using TabBar and slivers instead of the chips and ListViews for replies, mentions, messages
  • Adjust the mentions section to use the same CommentReference as the replies (messages will stay the same for now since it'll need to be overhauled)
  • Improves the fetching logic for inbox replies, mentions, and messages. It should now only fetch the appropriate information depending on the active tab. This should reduce API calls to the server since we're not unnecessarily fetching other information

Note: Because the underlying fetching logic was changed, we may need to do some quick testing on the push notifications page to make sure it still works as expected

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Here are some initial screenshots!

Replies Mentions
simulator_screenshot_7B4329DC-09FB-490A-8ED4-2D01E25125A4 simulator_screenshot_13A5F6AE-A2DE-451D-82C4-832D84C928F9

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu marked this pull request as ready for review July 2, 2024 17:38
@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 2, 2024

This should be ready to go now! I haven't tested out notifications since that's not readily available on iOS yet, but it should work as usual.

@micahmo Feel free to try it out and let me know if you find anything!

@micahmo
Copy link
Member

micahmo commented Jul 3, 2024

Sounds good, I think I will run this rather than doing an in-depth review and let you know if I find anything!

@micahmo
Copy link
Member

micahmo commented Jul 3, 2024

Hey just FYI, I am getting a gray screen exception when tapping on a notification. I can try to debug when I get a chance (not sure when that will be).

@micahmo
Copy link
Member

micahmo commented Jul 3, 2024

One more note: I'm not sure if you intended the "you've reached the bottom" message to have a gray background like it does for comments.

@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 3, 2024

I am getting a gray screen exception when tapping on a notification.

I can try this out on an Android emulator and see what's happening!

I'm not sure if you intended the "you've reached the bottom" message to have a gray background like it does for comments.

Hmm, I'm not sure what you're referring to - do you have a screenshot of this? The background should be transparent

Also, I noticed an issue where the inbox doesn't refresh properly when switching accounts from authenticated -> anonymous so I'll fix that as well!

@micahmo
Copy link
Member

micahmo commented Jul 4, 2024

I am getting a gray screen exception when tapping on a notification.

I can try this out on an Android emulator and see what's happening!

Thanks!

I'm not sure if you intended the "you've reached the bottom" message to have a gray background like it does for comments.

Hmm, I'm not sure what you're referring to - do you have a screenshot of this? The background should be transparent

I'm just referring to the difference in background color here. Also as I took these screenshots, I realized the text styling is different too. Maybe this should be a shared widget?

image image

P.S. I found one more thing. There doesn't appear to be a progress indicator when the inbox is being refreshed.

@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 4, 2024

I am getting a gray screen exception when tapping on a notification.

This should be fixed now!

I'm just referring to the difference in background color here. Also as I took these screenshots, I realized the text styling is different too. Maybe this should be a shared widget?

I adjusted this as well, so it should now match up with the feed reached end widget

@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 4, 2024

P.S. I found one more thing. There doesn't appear to be a progress indicator when the inbox is being refreshed.

Hmm, I'm seeing a refresh indicator when refreshing the inbox - can you clarify what you mean here?

refresh.webm

@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 5, 2024

I'll go ahead and merge this in so that it can be tested in the next nightly! Let me know if you encounter any more issues.

@hjiangsu hjiangsu merged commit 23f4aa2 into develop Jul 5, 2024
1 check passed
@hjiangsu hjiangsu deleted the refactor/inbox-page branch July 5, 2024 21:57
hjiangsu added a commit that referenced this pull request Jul 5, 2024
@micahmo
Copy link
Member

micahmo commented Jul 8, 2024

Hmm, I'm seeing a refresh indicator when refreshing the inbox - can you clarify what you mean here?

Yes, sorry, I think the problem is less about the lack of a refresh indicator and more that something is wrong after navigating back from the notifications page. You can see what I mean here.

qemu-system-x86_64_WkEZtaaDYl.mp4

@hjiangsu
Copy link
Member Author

hjiangsu commented Jul 8, 2024

Ahh I see, thanks for clarifying that! I'll take a look at it

@micahmo
Copy link
Member

micahmo commented Jul 9, 2024

Hey I noticed something else. Actually you can see it in the screen recording. Marking the last notification as read no longer closes the notification page. Not sure if that was intention and/or whether it was caused by your changes.

EDIT: In fact, marking posts as read doesn't hide them from the inbox page at all. I believe it's defaulting to "show all" rather than "show unread". Let me know if you want me to look into this since I might know the inbox page a little better (but I believe this change is due to relying on the shared inbox page widget from the notifications page).

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