Skip to content
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

QE: Share image #572

Open
wants to merge 19 commits into
base: trunk
Choose a base branch
from
Open

QE: Share image #572

wants to merge 19 commits into from

Conversation

pinarol
Copy link
Contributor

@pinarol pinarol commented Nov 21, 2024

Closes #566

Description

Opens a share sheet via the avatar menu. Downloads the image in full size before that. Otherwise the share sheet doesn't share the image. Also adding the URL because then the metadata is captured by the system and automatically displayed in the share sheet's navigation bar.

Discussion

I wonder how you feel about the medium detent for the share sheet. I added it to be able to test it, but not sure how I feel about it. Please also test the full size sheet by just commenting out .presentationDetentsIfAvailable([.medium, .large]) and compare.

Testing Steps

Test with both horizontal and vertical layouts.
Try different color schemes.
Test an error case like for example turn off internet.

Demo app > Quick Editor > Avatar ... menu > Share

@pinarol pinarol added enhance New feature or request [Feature] Gravatar-QuickEditor Gravatar Quick Editor [Priority] Medium labels Nov 21, 2024
@wpmobilebot
Copy link

wpmobilebot commented Nov 21, 2024

Gravatar Prototype Build📲 You can test the changes from this Pull Request in Gravatar Prototype Build by scanning the QR code below to install the corresponding build.
App NameGravatar Prototype Build Gravatar Prototype Build
Build Number1772
Version1.0
Bundle IDcom.automattic.gravatar-sdk-demo-uikit.prototype-build
Commit8821ead
App Center BuildGravatar SDK Demo - UIKit #411
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@pinarol pinarol marked this pull request as ready for review November 22, 2024 12:12
@@ -30,8 +31,7 @@ class AvatarPickerViewModel: ObservableObject {
@Published var selectedAvatarURL: URL?
@Published private(set) var backendSelectedAvatarURL: URL?
@Published private(set) var gridResponseStatus: Result<Void, Error>?

let grid: AvatarGridModel = .init(avatars: [])
@Published private(set) var grid: AvatarGridModel = .init(avatars: [])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been a @Published var from the beginning imo. Otherwise the UI can fail to get updated properly.

@andrewdmontgomery
Copy link
Contributor

⛏️ small adjustment

We will want to append an ellipsis to the "Share" menu item:

Share...

Append an ellipsis to a menu item’s label when the action requires more information before it can complete. The ellipsis character (…) signals that people need to input information or make additional choices, typically within another view.

@pinarol
Copy link
Contributor Author

pinarol commented Nov 22, 2024

We will want to append an ellipsis

Done.

Copy link
Contributor

@andrewdmontgomery andrewdmontgomery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the simulator, the context menu seems to work fine. But on a physical device, tapping the menu selects the image as an avatar. Once it is selected, then the menu is tappable

@pinarol
Copy link
Contributor Author

pinarol commented Nov 22, 2024

I test in the real device as well. Maybe the tapping area wasn't big enough for you and we need to make it a bit bigger 🤔

@pinarol
Copy link
Contributor Author

pinarol commented Nov 22, 2024

The problem seems to be related with iOS 17.x. Putting the Menu in the overlay doesn't seem to work there. I made a fix. Feel free to check again. @andrewdmontgomery and good catch 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhance New feature or request [Feature] Gravatar-QuickEditor Gravatar Quick Editor [Priority] Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share an image
3 participants