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

VtoC: Prototype #23205

Merged
merged 9 commits into from
May 23, 2024
Merged

VtoC: Prototype #23205

merged 9 commits into from
May 23, 2024

Conversation

kean
Copy link
Contributor

@kean kean commented May 14, 2024

Part of https://github.com/Automattic/wordpress-mobile/issues/102

To test:

  • Go through a happy path – ignore everything

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@kean kean force-pushed the task/integrate-jetpack-ai branch 2 times, most recently from 9094ff8 to 4e7cf01 Compare May 14, 2024 23:05
@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 14, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23205-50c0995
Version24.9
Bundle IDorg.wordpress.alpha
Commit50c0995
App Center BuildWPiOS - One-Offs #9975
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 14, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23205-50c0995
Version24.9
Bundle IDcom.jetpack.alpha
Commit50c0995
App Center Buildjetpack-installable-builds #9025
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

I understand that this is a prototype / a foundation to build on, so I haven't done a thorough test. The happy path seems to work well, but I noticed that if you record without saying anything, then tap "Done", the flow generates a paragraph block with the text "Thank you."

host.modalPresentationStyle = .formSheet
} else {
if let sheetController = host.sheetPresentationController {
sheetController.detents = [.medium()]
Copy link
Contributor

Choose a reason for hiding this comment

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

On smaller devices, "Post from audio" is obscured in the sheet

iPhone SE (17.4.1) iPhone 15 Pro (17.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest addressing it when we start working on the final design.

@@ -24,8 +24,7 @@ import WordPressUI
private let noticeAnimator = NoticeAnimator(duration: 0.5, springDampening: 0.7, springVelocity: 0.0)

private func notice(for blog: Blog) -> Notice {
let showsStories = blog.supports(.stories)
let title = showsStories ? NSLocalizedString("Create a post, page, or story", comment: "The tooltip title for the Floating Create Button") : NSLocalizedString("Creates new post, or page", comment: " Accessibility hint for create floating action button")
let title = NSLocalizedString("Creates new post, or page", comment: " Accessibility hint for create floating action button")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should use the same copy as showCreateButton - "Create a post or page"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point – I moved this to Stings too.

wpAssertionFailure("blog missing")
return
}
let viewModel = VoiceToContentViewModel(blog: blog) { [ weak self] transcription in
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]

Suggested change
let viewModel = VoiceToContentViewModel(blog: blog) { [ weak self] transcription in
let viewModel = VoiceToContentViewModel(blog: blog) { [weak self] transcription in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +59 to +62
Text("File size limit: 25 MB")
Text("Recording time limit: 5 minutes")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these hardcoded values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I added a todo in the latest PR.

var body: some View {
VStack(spacing: 34) {
VStack(spacing: 16) {
Text("Post from Audio")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make the strings in this file localizable strings? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, I can do this when I implement the updated design

@kean
Copy link
Contributor Author

kean commented May 22, 2024

if you record without saying anything, then tap "Done", the flow generates a paragraph block with the text "Thank you."

huh, good catch. I wonder if we can even do anything about it.

@kean kean force-pushed the task/integrate-jetpack-ai branch from 4e7cf01 to 50c0995 Compare May 22, 2024 19:08
@kean kean requested a review from momo-ozawa May 22, 2024 19:09
@kean kean merged commit 76db578 into trunk May 23, 2024
24 checks passed
@kean kean deleted the task/integrate-jetpack-ai branch May 23, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants