-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Gravatar Quick Editor as My Profile editor #24583
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
Gravatar Quick Editor as My Profile editor #24583
Conversation
| @objc private func gravatarButtonTapped() { | ||
| guard let email = gravatarEmail, | ||
| let presenter = GravatarQuickEditorPresenter(email: email), | ||
| let presentingViewController else { return } | ||
| presenter.presentQuickEditor(on: presentingViewController) | ||
| guard | ||
| let presenter = GravatarQuickEditorPresenter(), | ||
| let presentingViewController | ||
| else { return } | ||
| presenter.presentQuickEditor(on: presentingViewController, scope: .avatarPicker()) | ||
| } | ||
| } |
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 believe that this class and MyProfileViewController related types are no longer in use.
Should we clean up?
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 you could remove MyProfileViewController and the related types, that would be ideal.
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 27962 | |
| Version | PR #24583 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 78b9126 | |
| Installation URL | 705b1hlvgk5oo |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 27962 | |
| Version | PR #24583 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 78b9126 | |
| Installation URL | 2mq702na5nk5g |
kean
left a comment
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.
Hey, I tested the editor on iPhone, and it worked well.
There is a little bit of a UX issue due to a large, non-scrollable header that makes the form much smaller than it can be:
It's especially visible with accessibility fonts on:
The email field also sticks out with large fonts. I'd suggest displaying in inside the header "card" below the name.
I haven't tested the iPad version. I presume the form is shown in a "form" modal?
| @objc private func gravatarButtonTapped() { | ||
| guard let email = gravatarEmail, | ||
| let presenter = GravatarQuickEditorPresenter(email: email), | ||
| let presentingViewController else { return } | ||
| presenter.presentQuickEditor(on: presentingViewController) | ||
| guard | ||
| let presenter = GravatarQuickEditorPresenter(), | ||
| let presentingViewController | ||
| else { return } | ||
| presenter.presentQuickEditor(on: presentingViewController, scope: .avatarPicker()) | ||
| } | ||
| } |
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 you could remove MyProfileViewController and the related types, that would be ideal.
|
Hey folks, @nbradbury mentioned here that he likes combined avatarAndPicker scope. I think this is something that should be synced between iOS and Android. What are your thoughts about it? |
|
Thank you @kean for the review! We implemented a few changes taking into consideration reviews from Android and iOS PRs. Regarding to your feedback, we have:
Besides this, I have removed from the project the types which have become obsolete. We also wanted to mention that we have another alternative grid layout for the Avatar picker. Regarding iPad, yes, it will present as a form modal:
|
|
@kean - Ready for another look 👀 |
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
Minor comments:
-
I still find it strange that only a small portion of the screen is scrollable while the rest isn’t. Additionally, the scroll indicator seems too far from the right edge. I recommend making the entire screen scrollable, including the header. It would also be helpful to move the Gravatar logo elsewhere and place the "Save" button in the navigation bar. This change would help save vertical space and align with the standard design for these types of forms in the app. If this isn't the direction you want to take, that's okay, but it doesn't follow typical native app conventions.
-
The notice is shown too close to the keyboard
Hey, @crazytonyli , do you want to take a second look at this PR?
Modules/Package.swift
Outdated
| .package(url: "https://github.com/Automattic/Automattic-Tracks-iOS", from: "3.5.2"), | ||
| .package(url: "https://github.com/Automattic/AutomatticAbout-swift", from: "1.1.5"), | ||
| .package(url: "https://github.com/Automattic/Gravatar-SDK-iOS", from: "3.2.0"), | ||
| .package(url: "https://github.com/Automattic/Gravatar-SDK-iOS", revision: "ce04275c6237131576f424a215197bfaeaefc416"), |
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.
Just a reminder to update the commit to a tag.
|
Thank you all for the reviews! I have updated the reference to the latest release and will merge after CI gets ✅ 🙏 |
|






Description
This PR changes the previous
MyProfileViewControllerwith the Gravatar Quick Editor to edit the My Profile fields.Testing instructions
Consider testing the following yourself before requesting a review:
-->