-
Notifications
You must be signed in to change notification settings - Fork 121
[Bookings] Customer data for Booking Details #16214
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
[Bookings] Customer data for Booking Details #16214
Conversation
Generated by 🚫 Danger |
|
|
# Conflicts: # WooCommerce/Classes/ViewModels/Booking Details/BookingDetailsViewModel.swift # WooCommerce/WooCommerce.xcodeproj/project.pbxproj
itsmeichigo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with iPhone 17 simulator and confirmed that this works as expected. I left some nit-picking comments below.
Irrelevant to the PR: Please force the date and time displayed in the detail screen to use UTC like on the booking list as suggested in the thread p1759398245019489-slack-C09FHQNQERG. Currently, we're displaying different time on the two screens.
| import struct Networking.Booking | ||
| import struct Networking.Customer | ||
| import struct Networking.Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It's not necessary to import these models from Networking as they are exported from Yosemite in Model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in be1c572
| withAnimation { | ||
| sections.insert(customerSection, at: 2) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While running the app with Xcode, I saw a warning for this update: "Publishing changes from background threads is not allowed; make sure to publish values from the main thread (via operators like receive(on:)) on model updates." Please consider marking updateCustomerSection with @MainActor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in the other PR in 2fee1b9726fe4b46d0732e83134d29de90e860eb
| .foregroundStyle(Color.accentColor) | ||
| } | ||
| .padding(.vertical, Layout.rowTextVerticalPadding) | ||
| .tappable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the tap works only on image and text and not in the spacing in the middle. Please consider adding contentShape for the content in TappableViewModifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 396a379
| } | ||
| .padding(.vertical, Layout.rowTextVerticalPadding) | ||
| .tappable { | ||
| print("On phone ellipsis") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be handled in this PR or a subsequent one? It may be necessary to hide the call button if a device does not support calling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll implement that in a separate PR
| let notice = Notice(title: Localization.emailCopiedMessage, feedbackType: .success) | ||
| ServiceLocator.noticePresenter.enqueue(notice: notice) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We can also use the .notice modifier instead of using the singleton.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked in 7c2634e
a9ec0aa to
7c2634e
Compare

Part of: WOOMOB-1426
Description
Loads and displays customer data in Booking Details screen.
loadCustomerasCustomerActionthat fetches localStorage.Customerwith given site ID and customer ID.customerDetailsViewmethod with distinctCustomerDetailsViewfor better data binding and code organizing.BookingDetailsViewModel.CustomerContentas a model forCustomerDetailsViewas anObservableObjectwith@Publishedproperties.BookingDetailsViewModelimplements customer data local fetching and API syncing.Testing steps
customerIDto0in bookings response by using Proxyman.Testing information
Tested on iOS 26 iPhone 17 and iPad simulators.
Screenshots
RELEASE-NOTES.txtif necessary.