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

Custom fields: Update list screen to be shown in Order Details as part of current navigation #14059

Merged
merged 3 commits into from
Oct 1, 2024

Conversation

hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Sep 27, 2024

Description

Please do not merge until target is trunk

This quick PR updates the Custom Fields List screen to no longer appear as a modal, but instead as part of the existing navigation controller.

Check out this new behavior in iPhone and iPad:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-27.at.19.46.18.mp4
Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-09-27.at.19.45.51.mp4

Steps to reproduce

Build and go to Order Details, then try the Custom Fields button there. Ensure the navigation matches the video above, and that the List screen is not shown in a modal.

Also smoke test the navigation between Order Details <-> Custom Fields List <-> Custom Fields Editor and ensure everything works fine.

Testing notes

I tested the steps above in Simulator iPhone and iOS, as seen in the video.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

…stead of being presented modally.

Also updates the View to use NavigationStack
@hafizrahman hafizrahman changed the base branch from trunk to feat/13493-custom-fields-local-editing September 27, 2024 12:05
@dangermattic
Copy link
Collaborator

dangermattic commented Sep 27, 2024

1 Warning
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Sep 27, 2024

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14059-35ae02d
Version20.5
Bundle IDcom.automattic.alpha.woocommerce
Commit35ae02d
App Center BuildWooCommerce - Prototype Builds #11043
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@hafizrahman hafizrahman added the type: task An internally driven task. label Sep 27, 2024
@hafizrahman hafizrahman added this to the 20.7 milestone Sep 27, 2024
@hafizrahman hafizrahman marked this pull request as ready for review September 27, 2024 12:17
Copy link
Member

@pmusolino pmusolino left a comment

Choose a reason for hiding this comment

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

I'm not sure why, but after these changes, I see the cell with padding on the right and left sides, at least on iOS 18.

@hafizrahman
Copy link
Contributor Author

@pmusolino thanks for checking! I added 35ae02d to remove the padding and have updated the videos in the original post to reflect it. Can you check again and see if you still have the issue?

Base automatically changed from feat/13493-custom-fields-local-editing to trunk September 30, 2024 08:28
@pmusolino pmusolino self-assigned this Sep 30, 2024
@pmusolino pmusolino self-requested a review September 30, 2024 13:17
Copy link
Member

@pmusolino pmusolino left a comment

Choose a reason for hiding this comment

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

It works as expected.

However, presenting a navigation that is different from the original one creates a small glitch during navigation.
I suggest leaving a TODO note to remind us that this needs to be improved.

@hafizrahman
Copy link
Contributor Author

@pmusolino

However, presenting a navigation that is different from the original one creates a small glitch during navigation.

@itsmeichigo also raised something similar in #14068 (comment)

I agree with that, though, and in hindsight it would've been more consistent to copy Blaze's method of creating navigation coordinator (BlazeCampaignCreationCoordinator) and use UIKit navigation still. While I do find that the experience of setting up navigation using SwiftUI's NavigationStack to be more intuitive and concise, it's also unfortunate that connecting it with UIKit's navigation isn't as smooth as possible.

An improvement here would be to refactor the navigation to use coordinator. Given what's already set up, this would be a decently sized task, so let's keep this for now. In the meanwhile I'll add a bit more things like turning off animation and making the back icon bolder, to make the back navigation look less odd.

@hafizrahman hafizrahman merged commit 1767007 into trunk Oct 1, 2024
14 checks passed
@hafizrahman hafizrahman deleted the feat/13493-custom-fields-list-nav-update branch October 1, 2024 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants