-
Notifications
You must be signed in to change notification settings - Fork 293
Posting confirmation Toast #3015
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
base: master
Are you sure you want to change the base?
Posting confirmation Toast #3015
Conversation
Changelog-Added : Added a ToastView which provides confirmation to users when a Note has been posted Closes: damus-io#2083 Signed-off-by: SanjaySiddharth <[email protected]> Note : This is not complete . The UI part has only been worked on.
Changelog-Added : Added Global Listeners which can listen to the number of relays to which a event has been posted to Closes: damus-io#2083 Signed-off-by: SanjaySiddharth <[email protected]>
Added a toast which pops up when a user clicks on post button. Shows the list of relays to which the post has been successfully posted. Signed-off-by: SanjaySiddharth <[email protected]> Closes : damus-io#2083
Signed-off-by : SanjaySiddharth <[email protected]>
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.
Did a quick review to provide some general feedback, taking into account that this is a draft.
- The approach overall seems good!
- Added some suggestions on how toast views can be made more generic and enable different toast layouts.
- Added one comment about the color styling of the toast.
- I tried and it seems to be working. Good job!
Please let me know if you have any questions, or when you move this out of the draft stage, and you need a more formal review.
Thank you!
damus/Components/ToastView.swift
Outdated
RoundedRectangle(cornerRadius: 24) | ||
.fill(.white) | ||
) | ||
.shadow(color: .purple.opacity(0.35), radius: 6) |
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 think we can stick to more neutral colours for the toast itself (borders, shadows, and backgrounds) to better match the iOS look?
Two nice examples from iOS would be:
- The "AirPods connected" notification (Example image)
- The focus mode switch notification
cc @robagreda, any thoughts?
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.
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.
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.
@SanjaySiddharth, that is a good point! Perhaps we can try:
- Adding a subtle shadow (semi-transparent black, on the bottom of the notification)
- Adding a thin border (semi-transparent "adaptable black")
So it would look roughly like this:
What do you think?
damus/Components/ToastView.swift
Outdated
Text(message) | ||
.font(.caption) | ||
.fontWeight(.semibold) | ||
} |
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.
We can make the base ToastView
more generic and allow other usages to make toasts with custom layouts, by using ViewBuilders:
- https://swiftwithmajid.com/2019/12/18/the-power-of-viewbuilder-in-swiftui/#:~:text=Using%20@ViewBuilder
- There is another example in our codebase on
EventMutingContainerView.swift
![]() Hello @danieldaquino and @robagreda , Could you please advice on the UI ? The toast gives a glass like effect using :
|
This to me looks great, can we apply some shadow as well? that way it would look nicer + gives me the user some sense of elevation. Overall looks great! |
Hello @danieldaquino , how about this ? ![]() If this is fine, can we remove the progressView and I'll try changing this view to a ViewBuilder Thanks, |
SIgned-off-by: SanjaySiddharth<[email protected]>
Sorry for the delay on this, just saw this message. Looks great! |
Please let me know if you need anything from me. Thank you! |
Hello @danieldaquino , Sorry for taking up this much time.
|
Hi @SanjaySiddharth, no worries, thank you for asking! I was thinking of something like having a base "generic" toast view such as: import SwiftUI
struct ToastView<Content: View>: View {
let content: Content
init(@ViewBuilder content: () -> Content) {
self.content = content()
}
var body: some View {
HStack {
content
}
.padding()
.background(
RoundedRectangle(cornerRadius: 24)
.fill(.ultraThinMaterial)
.background(.white.opacity(0.05))
.overlay(
RoundedRectangle(cornerRadius: 24)
.stroke(Color.damusAdaptableBlack.opacity(0.2), lineWidth: 1)
)
.clipShape(RoundedRectangle(cornerRadius: 24))
.shadow(radius: 5.0, x: 0, y: 5)
)
.padding()
.transition(.opacity.combined(with: .move(edge: .top)))
.animation(.easeInOut(duration: 0.3), value: UUID()) // Use a unique value to trigger animation
}
} And then we make a specialized toast view for post confirmation using the base toast view. Example: struct PostConfirmationToastView: View {
var style: Style
var message: String
var completedEvents: Double = 1.0
var body: some View {
ToastView {
if let iconName = style.iconName {
Image(systemName: iconName)
.foregroundStyle(style.color)
} else {
ProgressView(value: completedEvents)
.progressViewStyle(.circular)
}
Text(message)
.font(.caption)
.fontWeight(.semibold)
}
}
enum Style {
(...)
}
} and the view modifier you created for post confirmation. In the end, ContentView { }
.postConfirmationToast(message: $postConfirmationToastMessage, style: .success) Mostly to separate what is a generic "toast view" and what is a specific toast view (e.g. "post confirmation toast view"), so that in the future we can use the generic toast view to create other toasts (e.g. "sending zap" toasts, etc). Something like that, but maybe we don't need to worry too much about it right now if it becomes complicated — we could also postpone this kind refactor for when we implement a second type of toast. Please let me know if this sounds good, if you prefer to postpone this, or if you have any other concerns/questions! Note: Code examples were generated with AI-assistance, I haven't thoroughly checked for the accuracy of that code. |
…asts Signed-off-by: SanjaySiddharth<[email protected]>
Hello @danieldaquino , This PR is ready for review . Let me know if any changes are required :) Thanks, |
Thank you @SanjaySiddharth. I am going to review this on Monday! |
can I see a video of this in action? |
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.
Thank you @SanjaySiddharth! I did a more detailed review (although I need to do a second pass later, I did not have time to go over all the details)
I added some specific comments in the code. Some general comments:
- Styling of the toast is looking good.
- There is some cleanup needed on the code and the commits. There are some formatting inconsistencies and unused code. I did not comment on all of them, but I added some examples on the comments.
- There seems a bit much margin between the top of the screen and the toasts? (I tested on an iPhone SE simulator)
- I noticed that toasts also get sent when reacting to posts. I think those will probably need custom messages as they are not "notes" — but I am not sure whether we need them. @jb55, any thoughts on whether we want toasts to appear when the user reacts to a
post? - We might benefit from having an animation/transition when the toast disappears.
- General code structure and approach seems good.
Please let me know if you need any help!
|
||
struct GenericToastView<Content:View>: View { | ||
// var style: ToastStyle | ||
// var message: String |
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.
We can delete unused code.
) | ||
.padding() | ||
.transition(.opacity.combined(with: .move(edge: .top))) | ||
// .animation(.easeInOut(duration: 0.3)) |
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.
Unused commented-out code.
|
||
import SwiftUI | ||
|
||
// Generic Toast View UI using which we build other custom Toasts |
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.
Thank for adding these documentation strings!
One tip (for next time, there is no need to fix it now), if you add this right on top of the struct
without an empty line, and use 3 slashes (///
), you can get this documentation to show up on the quick help or automated documentation generators.
A good article that talks about this: https://nshipster.com/swift-documentation/#documentation-is-my-new-bicycle
We are not super strict about code documentation though, so no need to worry — this is just for reference!
.stroke(Color.damusAdaptableBlack.opacity(0.2),lineWidth: 1) | ||
) | ||
.clipShape(RoundedRectangle(cornerRadius: 24)) | ||
.shadow(radius: 5.0, x:0 , y: 5) |
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.
Nitpick: There is some inconsistency in the way the code is formatted.
The convention is to add a space after the :
, and no space before the ,
is needed.
.shadow(radius: 5.0, x:0 , y: 5) | |
.shadow(radius: 5.0, x: 0, y: 5) |
.task { | ||
// for await (event, relayID) in relayNotification.stream { | ||
// postConfirmationToastMessage = "Your note has been posted to \(event.totalRelays-event.remaining.count) out of \(event.totalRelays) relays" | ||
// } |
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.
Unused code
for await notification in relayNotification.stream { | ||
switch notification { | ||
case .inProgress(let event, let relayID): | ||
postConfirmationToastMessage = "Your note has been posted to \(event.totalRelays-event.remaining.count) out of \(event.totalRelays) relays" |
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.
We should format this for localization to make sure it can be translated into different languages.
This one is a bit trickier than usual, but here is a useful comment from @tyiu from another recent PR that had a similar situation: #3031 (comment)
postConfirmationToastMessage = "Your note has been posted to \(event.totalRelays-event.remaining.count) out of \(event.totalRelays) relays" | ||
|
||
case .initial: | ||
postConfirmationToastMessage = "Your note is being posted..." |
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.
We can use NSLocalizedString
here:
postConfirmationToastMessage = "Your note is being posted..." | |
postConfirmationToastMessage = NSLocalizedString("Your note is being posted...", comment: "A label on a toast that indicates the user's post is being posted") |
This will ensure that our translators can translate this label to other languages.
// for await (event, relayID) in relayNotification.stream { | ||
// print("Rithi Event: Posted to \(event.totalRelays-event.remaining.count) out of \(event.totalRelays) relays") | ||
// } | ||
// } |
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.
Unused code
@@ -137,6 +145,11 @@ class PostBox { | |||
if ev.remaining.count == 0 { | |||
self.events.removeValue(forKey: event_id) | |||
} | |||
print("Toatl Relays : \(ev.totalRelays)") |
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.
Do we need this debug log? It looks like we might not need it.
@@ -137,6 +145,11 @@ class PostBox { | |||
if ev.remaining.count == 0 { | |||
self.events.removeValue(forKey: event_id) | |||
} | |||
print("Toatl Relays : \(ev.totalRelays)") | |||
|
|||
Task{ |
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.
Nitpick/Minor: Please use code formatting conventions for styling consistency. In this case, the convention is generally to use a space between Task
and {
Task{ | |
Task { |
Hello @danieldaquino , I will look into this in the weekend. Thank you ! |
Closes : #2083
Summary
This PR introduces Toasts which will provide the user with confirmation regarding posted notes.
This PR aims to provide the following posted confirmation info:
1. Number of relays the note has been posted to.
2. Success or Failure of posting
Checklist
Closes:
orFixes:
tags in the commit messages wherever applicable, or made sure those are not needed. See Submitting patchesTest report
Please provide a test report for the changes in this PR. You can use the template below, but feel free to modify it as needed.
Device: iPhone Simulatore
iOS: iOS 18.2
Damus: b35cc33
Results:
Other notes
This is a draft PR at present.