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.

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

Orders Search: User Interface #557

Merged
merged 13 commits into from
Jan 2, 2019
Merged

Orders Search: User Interface #557

merged 13 commits into from
Jan 2, 2019

Conversation

jleandroperez
Copy link
Collaborator

@jleandroperez jleandroperez commented Dec 26, 2018

Important:

  • The UI of this approach differs from the mockups.
  • Why: We didn't notice until we got hands on it, but having the SearchBar along with "Pull to Refresh" support leads to a quite poor UX: the user pulls to refresh, but ends up revealing the SearchBar, and must "pull further" to trigger the gesture.
  • I'm making a call, and replicating Contacts.app (Standard iOS UX), with a caveat: We're basically implementing the exact same feature, but instead of hiding the SearchBar, we'll rely on a NavigationBar Button to enter the "Search Mode".

Details:

  • Implemented OrderSearchViewController (barebones)
  • Relocated all of the Order-Y cells to a folder named Cells
  • Renamed: OrderListCell > OrderTableViewCell
  • Renamed: AddANoteViewController > NewNoteViewController

Ref. #63

Notes:

  • Cell Prototypes cannot be shared between multiple UIViewControllers within a Storyboard
  • Because of the above, it doesn't really make sense to have all the things in a storyboard. In this PR i've detached the NewNoteViewController from Orders.storyboard (along with OrderTableViewCell!)

Testing: Skeleton

  1. Fresh Install
  2. Log into your Store
  3. Open the Orders Tab
  • Verify the Skeleton UI looks correctly

Testing: New Note

  1. Log into your Store
  2. Open the Orders Tab
  3. Open any order's details
  4. Press over the New Note row
  • Verify the New Note UI looks good
  • Add a new Note
  • Verify the new Note shows up shortly onscreen

Testing: Search UI

  1. Log into your Store
  2. Open the Orders Tab
  3. Press over the Top Left icon
  • Verify the new UI shows up
  • Verify that the Cancel button dismisses the new UI

simulator screen shot - iphone se - 2018-12-26 at 12 18 31

simulator screen shot - iphone se - 2018-12-26 at 12 18 33

@jleandroperez jleandroperez added the feature: order list Related to the order list. label Dec 26, 2018
@jleandroperez jleandroperez added this to the 0.15 milestone Dec 26, 2018
@jleandroperez jleandroperez self-assigned this Dec 26, 2018
@wpmobilebot
Copy link
Collaborator

1 Warning
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 Danger

@jleandroperez jleandroperez mentioned this pull request Dec 26, 2018
12 tasks
@jleandroperez jleandroperez added this to Review/Testing in MVLP Kanban Board Dec 27, 2018
@mindgraffiti mindgraffiti mentioned this pull request Dec 27, 2018
8 tasks
@mindgraffiti
Copy link
Contributor

Crash on Test Search UI:
Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[WooCommerce.OrderSearchViewController tableView:numberOfRowsInSection:]: unrecognized selector sent to instance 0x12f91ba40'

Copy link
Contributor

@mindgraffiti mindgraffiti left a comment

Choose a reason for hiding this comment

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

Prototype Cells

thanks for taking care of this tech debt Jorge!

Note @bummytime the OrderSearchViewController is crashing because the tableview delegate methods aren't implemented in this PR --- they're in PR #559. I'm good with this getting merged if you are.

Copy link
Member

@bummytime bummytime left a comment

Choose a reason for hiding this comment

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

Looks good Jorge. As @mindgraffiti mentioned in her review:

... the OrderSearchViewController is crashing because the tableview delegate methods aren't implemented in this PR --- they're in PR #559. I'm good with this getting merged if you are.

I saw the same issue. I am also fine merging this one and testing it altogether in #559

Nice job! :shipit:

@bummytime bummytime merged commit c3bc3e9 into develop Jan 2, 2019
MVLP Kanban Board automation moved this from Review/Testing to Done/Merged Jan 2, 2019
@bummytime bummytime deleted the issue/63-orders-search branch January 2, 2019 17:58
@astralbodies astralbodies modified the milestones: 0.15, App Store release Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: order list Related to the order list.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants