Skip to content

Conversation

@hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Oct 21, 2024

Part of #13493

Description

This PR updates the navigation behavior for Custom Fields List and Custom Field Editor screens, as follows:

  • Previously, the List managed its toolbar using Swift UI for convenience. As it turned out, managing toolbar that way required some hacks as it is used by UI Kit-based screens (Order Details and Product Details), and it created some inconsistent design and behavior (more on that in this PR Custom fields: Entry point logic in Product Details #14068)
    With this PR, the List now has a hosting controller that also manages its toolbar. This way, the design and behavior consistency are preserved, and the hacks can be removed.
  • Toolbar buttons functionality in the List ("Save" and "+") is now handled by the hosting controller, and now involves the view model.
  • Entering the Editor now uses .sheet, to present it modally. This is consistent with an existing flow in Order Details > View Billing Information > Edit Billing Address., and with the suggestion proposed by @itsmeichigo here.
  • Navigation on "+" button is added, it will open the Editor in an empty state.
  • Since it becomes a modal, the Editor screen how has a "Cancel" button for closing it.

Steps to reproduce

  1. Have a test Order that contains Custom Field, then open its details in the app,
  2. Tap View Custom Fields. Ensure that the Custom Fields List screen is opened correctly
  3. Go back and forth between Order Details and Custom Fields List screen. Ensure that navigation works and the toolbar on the List screen looks correct,
  4. In the Fields List screen, tap an existing Custom Field. Ensure that the Custom Field Editor screen is shown correctly as a modal.
  5. Go back and forth between Fields List and Editor screens again. Ensure that navigation works and the toolbar on the Editor screen looks correct.
  6. In the Fields List screen, tap the plus button on top right. Ensure that the Custom Field Editor screen is shown correctly as a modal, with empty values.
  7. Test both in iPhone and iPad mode, as the Order Details layout differs. Check videos below for example.
  8. Repeat the same test also from Product Details. "Custom Fields" option appear right away if the Product already has them, or under the Add more details option otherwise.

Testing information

I tested this PR using simulator both iPhone and iPad version, following all the steps above.

Video

Phone mode:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-10-21.at.17.42.37.mp4

Tablet mode:

Simulator.Screen.Recording.-.iPad.Pro.13-inch.M4.-.2024-10-21.at.17.38.32.mp4

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.

@dangermattic
Copy link
Collaborator

dangermattic commented Oct 21, 2024

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is assigned to the milestone 20.9. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 21, 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 Numberpr14175-5b0bb45
Version20.8
Bundle IDcom.automattic.alpha.woocommerce
Commit5b0bb45
App Center BuildWooCommerce - Prototype Builds #11274
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 Oct 21, 2024
@hafizrahman hafizrahman added this to the 20.9 milestone Oct 21, 2024
@hafizrahman hafizrahman marked this pull request as ready for review October 21, 2024 10:52
Copy link
Contributor

@itsmeichigo itsmeichigo left a comment

Choose a reason for hiding this comment

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

Thank you for updating the navigation! I tested on both iPhone and iPad and it works as expected for both order and product custom field list 👍

I left some suggestions for code improvements, please check them out.

Button {
presentationMode.wrappedValue.dismiss()
} label: {
Text("Cancel") // todo-13493: set String to be translatable
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Any reason why we should not handle the localization right away?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 255d9ac

Just out of caution in case we want to update the text, which had happened several times in this project already.

Copy link
Contributor

@itsmeichigo itsmeichigo left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@hafizrahman hafizrahman merged commit 3aea5f1 into trunk Oct 24, 2024
14 checks passed
@hafizrahman hafizrahman deleted the task/13493-navigation-update-cf-list-details branch October 24, 2024 11:28
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.

6 participants