woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
258 stars 108 forks source link

Fulfillment screen: change edit icon to a delete icon #914

Closed ctarda closed 5 years ago

ctarda commented 5 years ago

Fix #877

A quick PR to update the "pencil" icon to a "delete" in FulfillViewController

Before and after icon

Changes

Testing

Update release notes:

ctarda commented 5 years ago

@mindgraffiti Thanks for the review. I definitely misunderstood the expected behaviour, will update this later to add the alert instead of showing the current UI

ctarda commented 5 years ago

@mindgraffiti @astralbodies I just pushed an update to present an alert and remove the existing UI to delete shipment trackings.

I gave a quick try at ex tracking the code to present the alert to a separate entity, but I think it'd be better to attempt that after implementing the alert again in the order details screen (that way it'd be easier to see exactly what can be extracted)

On the 11" iPad Pro it looks like this: Simulator Screen Shot - iPad Pro (11-inch) - 2019-04-24 at 09 22 44

On an iPhone 8 it looks like this: Simulator Screen Shot - iPhone 8 - 2019-04-24 at 09 24 54

astralbodies commented 5 years ago

I'm seeing a strange layout constraint warning when tapping the row/X for deleting on the iPhone XR simulator:

2019-04-24 09:28:41.970368-0500 WooCommerce[2993:96495] [LayoutConstraints] Unable to simultaneously satisfy constraints.
    Probably at least one of the constraints in the following list is one you don't want. 
    Try this: 
        (1) look at each constraint and try to figure out which you don't expect; 
        (2) find the code that added the unwanted constraint or constraints and fix it. 
(
    "<NSLayoutConstraint:0x6000018614f0 UIView:0x7fb53a469240.width == - 16   (active)>"
)

Will attempt to recover by breaking constraint 
<NSLayoutConstraint:0x6000018614f0 UIView:0x7fb53a469240.width == - 16   (active)>

Make a symbolic breakpoint at UIViewAlertForUnsatisfiableConstraints to catch this in the debugger.
The methods in the UIConstraintBasedLayoutDebugging category on UIView listed in <UIKitCore/UIView.h> may also be helpful.
astralbodies commented 5 years ago

Is it possible to make the X tap-able in itself like a detail disclosure indicator button would be? Also it might make sense to maybe vertically center it in the cell. @kyleaparker - thoughts?

peril-woocommerce[bot] commented 5 years ago
Warnings
:warning: PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS

ctarda commented 5 years ago

Thanks @astralbodies I pushed an update to ignore taps on the cell and only process those on the icon.

Regarding the Autolayout error, I completely missed that. I have been playing with it and I've noticed that we have the same errors, for example, when presenting an alert after tapping the ellipsis button next to the phone number in the Order Details screen. I'll see if I can come up with a solution to both errors

kyleaparker commented 5 years ago

Is it possible to make the X tap-able in itself like a detail disclosure indicator button would be? Also it might make sense to maybe vertically center it in the cell. @kyleaparker - thoughts?

Yep I would suggest we vertically center in the cell

astralbodies commented 5 years ago

@ctarda I took the liberty of changing the delete button to an accessory view for the table view cell. Since it's a UIButton it doesn't need the gesture recognizer and it also has a touch down state change that users would expect for a button. This also centers the image vertically in the cell.

Is that change okay? I'm not sure if that breaks patterns we follow elsewhere...

2019-04-25_14-35-03 (1)

I also simplified the popover presentation rectangle calculation logic.

kyleaparker commented 5 years ago

Another small tweak, can we reduce the space between the "Add tracking" button and the existing tracking codes to 16pt:

Woo MVP Android-iOS i8 20190218 2019-04-25 15-44-22
mindgraffiti commented 5 years ago

Thank you for all the hard work on this @ctarda and @astralbodies :)

mindgraffiti commented 5 years ago

Another small tweak, can we reduce the space between the "Add tracking" button and the existing tracking codes to 16pt:

@kyleaparker that's going to take a couple of hours to complete. @ctarda is using our custom header view to create a 2-column layout (to support a section header that can say "Product" and "Qty" in it. It's an autolayout issue with the custom header view. Last time I researched it, the header view height needs calculated manually and a new height constraint added. Bug filed #926.

ctarda commented 5 years ago

Thanks @astralbodies @mindgraffiti and @kyleaparker ! I'll merge as soon as the build passes