-
Notifications
You must be signed in to change notification settings - Fork 108
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
[Tablet Orders] Order list UI updates #12214
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
The current Woo colors might be out of date across the board, so we’re reverting the specific change on this commit. More details in the PR description.
@@ -439,20 +449,23 @@ extension EmptyStateViewController { | |||
|
|||
fileprivate var message: NSAttributedString { | |||
switch self { | |||
case .simpleTextWithDescription: | |||
return NSAttributedString(string: "") |
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.
We cannot break here without retuning something, and making the message an optional NSAttributedString
involves a bunch of changes everywhere we use the message
. Returning an empty attributed string that is not being used seemed to fit our case without complicating it too much.
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.
Consider mentioning this in a comment to save someone else refactoring unneccesarily
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: abf24bd
The two types of empty state for the list/details panes look a bit rubbish next to each other. Let's try to update both, at least as a follow up issue... |
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.
Not sure this one's ready, we can keep working on it after release...
func configureBordersWhenHighlighted() { | ||
let topBorder = CALayer() | ||
let bottomBorder = CALayer() | ||
let borderWidth = 0.5 | ||
|
||
topBorder.frame = CGRect(x: 0, y: 0, width: frame.width, height: borderWidth) | ||
bottomBorder.frame = CGRect(x: 0, y: 0, width: frame.width, height: borderWidth) | ||
topBorder.backgroundColor = UIColor.divider.cgColor | ||
layer.addSublayer(topBorder) | ||
layer.addSublayer(bottomBorder) | ||
} |
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.
These dividers don't quite look right, e.g. they're full width, and in this screenshot, one is placed over the default divider:
Given that only the selected cell dividers are missing, I think it's better to leave the existing behaviour than apply this change. There's probably a better way to do it...
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.
The fix should probably be in UITableViewCell.updateDefaultBackgroundConfiguration(using:style:)
so we get it everywhere...
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.
The fix should probably be in UITableViewCell.updateDefaultBackgroundConfiguration(using:style:) so we get it everywhere...
Yeah. I experimented with different approaches and the simplest one seems to be using the .stroke...
properties inside the cell configuration. Here's the change: ffed3e9
I wasn't able to find a good way to apply this change only to top and bottom:
- If we use CALayer we have to also play with removing them after a cell stops being selected/highlighted, which I wasn't able to do without crashing when populating the cells. This also adds additional unexpected lines around the first order and we'll have to add additional handling most likely by overriding
layoutSubviews
- Using the customView property feels a bit overkill to add these borders, at least for the moment. If we happen to redo the cells across the app this could make more sense.
The downside of the approach is that we can see the lateral border a bit wider when a cell is selected, but I think it makes sense and overall is an improvement:
// Adds a border to the top of tableview | ||
let topBorder = CALayer() | ||
let borderWidth = 0.5 | ||
topBorder.frame = CGRect(x: 0, y: 0, width: tableView.frame.size.width, height: borderWidth) | ||
topBorder.backgroundColor = UIColor.divider.cgColor | ||
tableView.layer.addSublayer(topBorder) |
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.
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.
Updated! 03f5cb6
@@ -439,20 +449,23 @@ extension EmptyStateViewController { | |||
|
|||
fileprivate var message: NSAttributedString { | |||
switch self { | |||
case .simpleTextWithDescription: | |||
return NSAttributedString(string: "") |
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.
Consider mentioning this in a comment to save someone else refactoring unneccesarily
Hey @joshheald, just acknowledging that I’ve seen this and will get back to you with something for the empty state next week. Let me know if you need it sooner. |
👋 @joe-keenan did you had the chance to look into this? Don't worry if not, as Josh mentioned is very low priority, I'm just catching up with this issue so I wanted to be sure before I update the existing ones. |
@joshheald this is ready for review when you have the time: Adressed:
Not addressed:
|
Hey @iamgabrielma, sorry about the delay. I was about to say I hadn’t done anything for this, but then I realised I had made a similar empty state for Analytics a while back. I reused the format and illustration to fit the orders list. I also feel like the Clear Filters CTA overwhelms the error message, so I changed it to a secondary button style. Let me know if you have any issues with it! 6kkCwI9VOEYcidp6UT5bln-fi-1725_29745 |
Thanks! Looks good! I tried to update this today but requires a bit more work than I expected. Eg:
With the current focus I'll move this latest update to the issue that is already open on #12625 for empty states, so we can merge the order improvements for the time being. I haven't made any further changes since the latest update, so the addressed vs not addressed items remains the same 👍 |
That’s fine. I also would like a more cohesive approach to error states. Thanks for creating a separate issue for it. |
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.
Thanks for the changes, they look good to me 👍
Partially closes #12149
Description
This PR addresses most of the UI issues from the design review regarding the Order list. We have left outside of this PR those changes related to semantic colours, since all might need to be reviewed as per p1709628907203209/1708465836.162389-slack-C0354HSNUJH . This will be prioritized and tackled separately.
Changes
OrderListTableViewController
Testing instructions
sideBySideViewForOrderForm
enabled, on a tablet or large iPhone.