Closed ctarda closed 5 years ago
@ctarda re:
[...] shipment data will be presented only for fulfilled orders, pending further discussion ( p1551231496200300-slack-C6H8C3G23 )
Check out my comment in that Slack thread (which may not matter, but just making sure 😄 )
@ctarda FYI, I was able to get the resultsController
wired up to your UI in a test branch. Just a few small changes to get everything working: https://github.com/woocommerce/woocommerce-ios/compare/issue/250-tracking-ui...try/250-tracking-ui-live-data
Also, the reason the tracking data wasn't loading unless the order was Completed
is because of this line. The guard
statement had viewModel.isProcessingPayment == false
and that is not needed in this situation.
Thanks @bummytime and @mindgraffiti for your comments. I incorporated everything, with one exception: Hound does not seem to like the two white lines before an extension. ~I will try to look into how Hound works and see if there is any way to allow that convention~. I just noticed there is a PR for that already
Other than that, now the view controller is fully wired with the ShipmentTracking store, and it is presenting actual data fetched from the backend.
I will try to play with ghosts cells around today to see if I can improve the presentation a bit, but in the meantime, I think this is ready for a proper review
I played with the ghosts for a bit and couldn't make any real improvements. I am not sure I really understand how that works.
3. Update the UI to match the I7 designs — the layout/style of the tracking cells have changed a tad:
The buttons were updated to include less prominent links as it can get a bit overwhelming when there are multiple shipments in the list. Still thinking about whether it makes sense to lead with the Provider or Shipping number (Example with provider first). Any thoughts on this?
@kyleaparker As a user, I think I would prefer leading with the provider. That way it would be easier for me to scan which of the possible shipments I really want to track
Warnings | |
---|---|
:warning: | PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible. |
Generated by :no_entry_sign: dangerJS
I've updated the cell to (hopefully) match the i7 design.
This is how it looks like now:
Notice how the shipment date is localised (to the left, English, US Region, to the right English HK Region)
@bummytime @mindgraffiti I had to make a few minor changes to the cell to accommodate an extra label. The changes are encapsulated in commit cf6e4ffd if you could take a look at the code that would be 💯
@kyleaparker as discussed, we are leading with the provider, let me know how it looks! 😃
Looks good! Can we just extend the line after the shipment tracking link to the edge:
Thanks @kyleaparker ! I've just pushed an update to make the separators extend to the edges:
Thanks @mindgraffiti and @bummytime !
I pushed an update to incorporate your latest comments, including hiding the tracking button when there is no valid tracking url. This is how it would look:
What do you think @kyleaparker ?
@ctarda I like your updates! FYI there is a small issue I discovered this morning. The tracking web extension allows you to enter a URL like so:
(no http://
or https://
prefix)
When the app receives this URL and the user taps "Track package" an error is thrown because the string cannot be converted to a URL:
2019-03-06 11:07:40.597859-0600 WooCommerce[1653:24075825] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: 'The specified URL has an unsupported scheme. Only HTTP and HTTPS URLs are supported.'
We should probably handle error.
Looks good, just one request is to have the padding match up at the top and bottom of the tracking details:
I just pushed another update. @bummytime I went with prepending a https
to urls that do not start with http or https. I am not sure if that is the best option. I also considered hiding the track button for those urls that do not start with a valid scheme. Let me know what you think and I'll update accordingly
@kyleaparker I updated the top padding, now it looks like this:
@ctarda I like the attempt to "prepend a https to urls that do not start with http or https" option because it assumes the best intention of our users. It might not be optimal for us but if something breaks and the user looks at the URL, they can tell if it was malformed, which leaves better clues compared to not linking at all.
If the event tracking that @bummytime mentioned is completed, let's get this shipped 😀
@kyleaparker there is a problem with downloading the Sketch file. When the Sketch file is available, we can correct the cell layout in this separate task: https://github.com/woocommerce/woocommerce-ios/issues/745
@ctarda @mindgraffiti I have fixed the problem with the Sketch file, you should be able to download it now 😄
If the event tracking that bummytime mentioned is completed, let's get this shipped 😀
Completely agree! 🎉
Thanks @mindgraffiti @bummytime @kyleaparker I will merge now and updated the cell according to the Sketch file tomorrow morning.
Ref: #250
This PR adds UI elements and logic to present Shipment Tracking in the
OrderDetailsViewController
.At this moment, even though this PR includes the code to integrate with Yosemite to fetch tracking data, the data presented on screen is mocked until I figure out why that integration is not working (for me) as expected. ( p1551246523204900-slack-C6H8C3G23 )
Testing
As mentioned, at the moment the shipment data is mocked. Also, shipment data will be presented only for fulfilled orders, pending further discussion ( p1551231496200300-slack-C6H8C3G23 )
Make sure you have a fulfilled order with one (or more) tracking numbers:
Check out the branch
Navigate to Orders > The order setup with tracking. Notice if mock tracking data is presented:
Mock data will be removed and testing instructions will be updated when this PR is ready for its final review