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.

Sign up for GitHub

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

Jump to bottom

Custom Fields: Prepare list view and viewmodel to be reusable #13639

Merged
merged 8 commits into from
Aug 21, 2024
Merged

Custom Fields: Prepare list view and viewmodel to be reusable #13639

merged 8 commits into from
Aug 21, 2024

Conversation

hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Aug 16, 2024 •
edited by itsmeichigo
Loading

Part of: #13493

Description

This PR mainly updates OrderCustomFieldsDetails and its related viewmodel to be CustomFieldsDetails by renaming them, moving the path out of Order, and generally making it to be usable by Products later on, too.

Additionally this also adds EditableCustomFieldRow, which is a new design for each custom field row, gated behind feature flag. It's intentionally similar to the original field, just limiting the value to three lines max, and adding a chevron. The design is still under discussion so it doesn't need too much attention for now, it just needs to be there for future uses.

Steps to reproduce

  1. Prepare an Order that contains some custom fields in it,
  2. Run app,
  3. Go to Orders,
  4. Open the Order that has custom fields in it,
  5. Tap the "View Custom Fields" button,
  6. Make sure the fields are shown properly,
  7. Make sure nothing looks glaringly wrong in the design:

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 Aug 16, 2024 •
edited
Loading

1 Warning
⚠️ This PR is assigned to the milestone 20.1. 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 Aug 16, 2024 •
edited
Loading

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 Numberpr13639-1e1819b
Version20.0
Bundle IDcom.automattic.alpha.woocommerce
Commit1e1819b
App Center BuildWooCommerce - Prototype Builds #10480
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@hafizrahman hafizrahman changed the title Issue/13493 custom fields list reusability Custom Fields: Make list view and viewmodel reusable Aug 16, 2024
@hafizrahman hafizrahman added the feature: order details Related to order details. label Aug 16, 2024
@hafizrahman hafizrahman added this to the 20.1 milestone Aug 16, 2024
@hafizrahman hafizrahman changed the title Custom Fields: Make list view and viewmodel reusable Custom Fields: Prepare list view and viewmodel to be reusable Aug 16, 2024
@hafizrahman hafizrahman marked this pull request as ready for review August 16, 2024 12:42
@itsmeichigo itsmeichigo self-assigned this Aug 19, 2024
itsmeichigo
itsmeichigo reviewed Aug 19, 2024
View reviewed changes
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.

Since the UI is essentially the same for the custom field row except for the line limit and the chevron, can we just use the same view but add a bool to update the view as needed?

I also left a few suggestions in the comments.

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsDetails.swift Outdated
@@ -1,19 +1,28 @@
import SwiftUI

struct OrderCustomFieldsDetails: View {
struct CustomFieldsDetails: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Please rename the view to clarify that it's a view. The postfix View is usually used for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, updated in 27ec9b5

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsDetails.swift Outdated
@Environment(\.presentationMode) var presentationMode

let customFields: [OrderCustomFieldsViewModel]
let viewEditCustomFieldsInProductsAndOrdersEnabled: Bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: the naming is a bit verbose making it hard to follow. How about isEditable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in e23c6d2

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsDetails.swift Outdated
Comment on lines 101 to 102
.padding([.top, .bottom], Constants.hStackPadding)
.padding(.horizontal, Constants.hStackPadding)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this can be combined into .padding(Constants.hStackPadding) to set the same padding for all edges.

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 as part of 1706a3f

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsDetails.swift Outdated
Comment on lines 161 to 162
.padding([.top, .bottom], Constants.hStackPadding)
.padding(.horizontal, Constants.hStackPadding)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above about padding.

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 as part of 1706a3f

WooCommerce/Classes/ViewRelated/Custom Fields/CustomFieldsDetails.swift Outdated
Comment on lines 186 to 193
private extension EditableCustomFieldRow {
enum Constants {
static let spacing: CGFloat = 8
static let vStackPadding: CGFloat = 16
static let hStackPadding: CGFloat = 10
static let height: CGFloat = 64
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: instead of duplicating the constants, you can bring the enum to CustomFieldsDetails extension and reuse it for both CustomFieldRow and EditableCustomFieldRow.

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 as part of 1706a3f

@hafizrahman
Copy link
Contributor Author

Since the UI is essentially the same for the custom field row except for the line limit and the chevron, can we just use the same view but add a bool to update the view as needed?

That makes sense. I was thinking that it's simpler to reason about when there's a separate view, instead of having to check for multiple bool checks, but I think in this case it's not too complicated. Added in 1706a3f

@hafizrahman
Merge branch 'trunk' into issue/13493-custom-fields-list-reusability
1e1819b 
* trunk: (115 commits)
  Fix typo in System Status Report
  Add barcode scanner icon update to 20.1 release notes
  Update to SF Symbols barcode scanner icon
  Rename text field variables and improve comments with the explanation
  Update naming
  Remove unnecessary keyboard focusing on PercentageView appear
  Revert unnecessary naming changes
  Added comment for checkIfNavigationBarShouldPop
  Fix unit test failure
  Fix unit test failures
  Check if navigation item is exact one
  Remove parameters for checkIfNavigationBarShouldPop
  Call super.shouldPopOnBackButton()
  Add order discard confirmation to 20.1 release notes
  Bump version number
  Update metadata translations
  Update app translations – `Localizable.strings`
  Fix compiler merge issues
  Fix compiler merge issues
  Keep ItemListView in the same state when doing pull to refresh
  ...
itsmeichigo
itsmeichigo approved these changes Aug 21, 2024
View reviewed changes
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 - LGTM :shipit:

@hafizrahman hafizrahman merged commit 3e17635 into trunk Aug 21, 2024
22 checks passed
@hafizrahman hafizrahman deleted the issue/13493-custom-fields-list-reusability branch August 21, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itsmeichigo itsmeichigo itsmeichigo approved these changes

@selanthiraiyan selanthiraiyan Awaiting requested review from selanthiraiyan

Assignees

@itsmeichigo itsmeichigo

Labels
feature: order details Related to order details.
Projects
None yet
Milestone
20.1
Development

Successfully merging this pull request may close these issues.
4 participants
@hafizrahman @dangermattic @wpmobilebot @itsmeichigo