Merge Ratings into AtlText #595
Conversation
This reverts commit 242ccf1.
Generated by 🚫 Danger |
|
 |
App Name |  Gravatar Prototype Build |
| Build Number | 1853 | |
| Version | 1.0 | |
| Bundle ID | com.automattic.gravatar-sdk-demo-uikit.prototype-build | |
| Commit | 175f490 | |
| App Center Build | Gravatar SDK Demo - UIKit #460 |
| package func update(_ altText: String?, rating: AvatarRating?, avatarID: String, accessToken: String) async throws { | ||
| @discardableResult | ||
| package func update( | ||
| _ altText: String?, |
There was a problem hiding this comment.
| _ altText: String?, | |
| altText: String?, |
Now that we have multiple parameters that we can "update", I think the calling site might read better if we name the first parameter. What do you think?
There was a problem hiding this comment.
Let's go with this change 👍
| rating: AvatarRating?, | ||
| avatarID: String, | ||
| accessToken: String | ||
| ) async throws -> Avatar { |
There was a problem hiding this comment.
We don't currently use this return data. But since the API defines a return type for this, I thought we probably should return it, and make it @discardableRersult.
That does mean that if we can't decode it (which I would expect to be quite rare since it would be a backend error that breaks the API), this will throw an error.
What do you think?
There was a problem hiding this comment.
Eventually, if (when?) we make this part of the API public, we can follow the pattern of:
public update(::::) --> AvatarType
package update(::::) --> Avatar| @@ -337,6 +338,7 @@ class AvatarPickerViewModel: ObservableObject { | |||
| await profile | |||
| } | |||
|
|
|||
| @discardableResult | |||
There was a problem hiding this comment.
Reasonable? Or do we want to check the result?
| rating: AvatarRating?, | ||
| avatarID: String, | ||
| accessToken: String | ||
| ) async throws -> Avatar { |
| package func update(_ altText: String?, rating: AvatarRating?, avatarID: String, accessToken: String) async throws { | ||
| @discardableResult | ||
| package func update( | ||
| _ altText: String?, |
There was a problem hiding this comment.
Let's go with this change 👍
andrewdmontgomery
merged commit 86d708c
into
etoledom/alt-text-editor
andrewdmontgomery
deleted the
andrewdmontgomery/etoledom/alt-text-editor
branch
Description
Now that #589 (QE: Avatar Rating) PR just merged into
trunk, this mergestrunkback into #590 (AltText Editor).trunk(including the new Avatar Rating capability).altTextinto the new Menu structuresetRating(_ rating: AvatarRating, for avatar: AvatarImageModel) async-->update(_ avatar: AvatarImageModel, rating: AvatarRating) async -> Boolupdate()functions@discardableResultAvatarService.update(::::)AvatarPickerViewModel.update(_: altText)AvatarPickerViewModel.update(_: rating)ProfileService.setRating(:::)function in favor of theAvatarService.update(::::)function in the AltText PRTesting Steps