-
Notifications
You must be signed in to change notification settings - Fork 201
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
Removed problematic toast (was not specified in Figma for this view) #657
Conversation
Probably, you can remove toast value from state. |
@shin-usu Fixed as suggested. Thanks! |
It seems like build error is occurring 👀 |
@shin-usu Sorry about that. This should fix that build issue. Tested again locally and it seems to be fine. |
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.
@charles-b-stb
Thank you for your commit!
I left some comments. Please check and response🙏🏻
@@ -103,7 +103,7 @@ public struct TimetableReducer : Sendable{ | |||
return item.isFavorited | |||
})) | |||
} | |||
case let .favoriteResponse(.success(isFavorited)): | |||
case .favoriteResponse(.success(_)): |
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.
If isFavorited is unnecessary, favoriteResponse case's type should be modified.
For example.
public enum Action : Sendable, BindableAction {
...
case favoriteResponse(Result<Void, any Error>)
...
}
switch action {
...
case .favoriteResponse(.success):
return .none
...
}
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.
fixed
state.toast = .init(text: String(localized: "AddFavorite", bundle: .module)) | ||
} | ||
case .favoriteResponse(.success(_)): | ||
//No response here on purpose. Toasts look bad here. |
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 this comment is unnecessary since toast does not exist in Reducer and View anymore.
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.
Fixed
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.
LGTM🎉🎉 Thanks!!
Issue
Overview (Required)
Links
Screenshot (Optional if screenshot test is present or unrelated to UI)
Movie (Optional)