-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add "Change Avatar" button to account settings #1770
Conversation
Wouldn't it make more sence to just tap the icon? |
I think this is an experimental feature that's why, it's not a fully native experience to begin with. We just open a browser to change the avatar. But it's a fair question. cc @Luchadores and @david-gonzalez-a8c if you want to add anything. |
I think we can try it, not sure if there's any other trigger that Pocket Casts already has on the Avatar itself. If not, we can do it, and see if users tap more directly the Avatar itself. |
eec048c
to
7f05c4d
Compare
7eebc46
to
43aeef4
Compare
43aeef4
to
7d8b730
Compare
We did some discussions and for now we'll move fwd with the "Change Avatar" button. |
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.
@pinarol from the code side, all looks good. The only thing missing is updating the CHANGELOG
.
I have a few questions/suggestions though. I understand that many of them are out of the scope of this task — and it involves changes in web/design. My main goal is just to make the experience as good as possible:
- The email field has autocomplete and autocorrect on. I think we might want to disable those as it kinda keeps suggesting changes to email:
-
Do we need to enter the email? We already have this information, in theory, we could be directly redirected to the code phase, without requiring the user to type any info. This will likely reduce friction.
-
Not a big deal, but the icon feels a bit different from the rest to me. The border seems slightly thinner and the inside normally has a more subtle blue.
- I felt an "avatar changed" confirmation is missing (either on the webpage or in the app). After cropping the image, there's no success message. I wonder if for any new user, this will confuse them as to whether they changed the avatar or not.
I don't think any of those comments are blockers but if we tackle them the experience will be smooth IMHO. Anyway, the only blocker is updating the CHANGELOG
.
let safariViewController = GravatarSafariViewController(destination: .avatarUpdate(email: email)) else { return } | ||
safariViewController.modalPresentationStyle = .automatic | ||
present(safariViewController, animated: true) | ||
Analytics.track(.accountDetailsChangeAvatar) |
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 need to update this spreadsheet when adding new events. I went ahead and created the entry for this one.
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! 🙇
I'll fwd these feedbacks to @wellyshen @aaronfc
I agree, it would be better if those are disabled.
I think the confirmation is there but its position isn't right so it's not easily viewable to user. Seems like a bug. issues created: https://github.a8c.com/Automattic/gravatar/issues/108240 https://github.a8c.com/Automattic/gravatar/issues/108241 |
Unfortunately, we need the user to enter their email for now. In Gravatar, users can define multiple avatars for multiple emails under one account. But they can log in to Gravatar only with their primary email. If the user is using one of those secondary emails in Pocket Casts, then pre-filling the field would be misleading because the login attempt would fail. But there's an ongoing project to update the login flow to allow logins by secondary emails. After that the pre-filling can also be an option I think. |
cc: @Luchadores this is a feedback about the new change avatar icon. |
Hey, sorry to jump in. But this is not accurate. You can (and you should) pass the The correct URL, I believe, should be |
I am applying the new icon. Does it look ok @david-gonzalez-a8c ? ![]() sharing the developer view also, the size differences are related with inner insets of the icons: ![]() |
Hey Pinar, before David replies, that looks nice, but we don't have the different hues in the icon like the rest. Sync with @etoledom, I believe he solved that last time. Attached is a screenshot of the previous icon we used, but you can see the difference as it plays on the blue opacity color. ![]() |
That's because the icon from the library doesn't have different opacities as other icons have (for example). I've just updated the icon in the Android PR, but let me know if you should use another one. |
@hamorillo is right. For that effect to happen the icon should have different opacities in it. @david-gonzalez-a8c could you check the icon and share an updated version if you think it doesn't look right #1770 (comment) ? |
Apart from the icon, this is ready for another look. @leandroalonso @danielebogo |
@@ -170,6 +175,8 @@ public enum FeatureFlag: String, CaseIterable { | |||
"default_player_filter_callback_fix" | |||
case .upNextOnTabBar: | |||
"up_next_on_tab_bar" | |||
case .gravatarChangeAvatar: | |||
"gravatar_change_avatar" |
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.
Let me know if this is the right thing to do to make the FF controllable remotely.
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.
Nit: In theory if the value doesn't change, you don't need to specify the lower snake case as it will be automatically converted below by rawValue.lowerSnakeCased()
@@ -139,6 +142,8 @@ public enum FeatureFlag: String, CaseIterable { | |||
false | |||
case .transcripts: | |||
false | |||
case .gravatarChangeAvatar: | |||
true |
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 made it true by default, let me know if you think it needs to be false.
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.
@bjtitus correct me if I'm wrong, but in theory we can switch off a FF if the default value is true
right?
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.
Yes, that's right. This value is just used to set the initial value so that it's enabled or disabled prior to connecting to Firebase. We also tend to leave this value false
while testing until we are mostly sure we want to release in whichever version this is going into.
👋 I recreated that specific icon with transparency. Attached is an SVG, and PNGs (1x, 2x, 3x). Hope it helps 🤞 |
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.
@@ -170,6 +175,8 @@ public enum FeatureFlag: String, CaseIterable { | |||
"default_player_filter_callback_fix" | |||
case .upNextOnTabBar: | |||
"up_next_on_tab_bar" | |||
case .gravatarChangeAvatar: | |||
"gravatar_change_avatar" |
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.
Nit: In theory if the value doesn't change, you don't need to specify the lower snake case as it will be automatically converted below by rawValue.lowerSnakeCased()
@@ -139,6 +142,8 @@ public enum FeatureFlag: String, CaseIterable { | |||
false | |||
case .transcripts: | |||
false | |||
case .gravatarChangeAvatar: | |||
true |
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.
@bjtitus correct me if I'm wrong, but in theory we can switch off a FF if the default value is true
right?
Yes that's the case now since we introduced a kind of cache buster parameter with this commit. 04d8c08. |
Co-authored-by: Brandon Titus <[email protected]>
I think we are ready to merge. 🎉 Thank you all for your help and feedback. 🙇 |
We need 👍 from @leandroalonso as well. |
Fixes # gravatarp2 -> 2024/06/10/pt-integrating-gravatar-into-pocket-casts
Adding the ability to change the avatar. Avatar is coming from user's Gravatar account. So the "Change Avatar" button opens the Gravatar web link via in-app Safari.
To test
Profile > Account > "Change Avatar"
Observe: In app Safari is presented
If you are not logged in to the app then "Change Avatar" button will not be present for you.
Known issue:
A strange auto-zoom happens when testing in a Simulator but this issue is not reproduced in real device. But let me know if you can reproduce this in a real device.
Checklist
CHANGELOG.md
if necessary.