-
Notifications
You must be signed in to change notification settings - Fork 135
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
Issue/11508 coupons card 2 #11543
Issue/11508 coupons card 2 #11543
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11543 +/- ##
============================================
- Coverage 40.33% 40.23% -0.10%
Complexity 5200 5200
============================================
Files 1087 1087
Lines 63224 63371 +147
Branches 8667 8685 +18
============================================
Hits 25499 25499
- Misses 35430 35577 +147
Partials 2295 2295 ☔ View full report in Codecov by Sentry. |
tint = colorResource(id = R.color.action_menu_fg_selector) | ||
tint = MaterialTheme.colors.primary |
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.
Just an inconsistency that I noticed, we were using the wrong color for the menu button here.
navController.navigateSafely( | ||
DashboardFragmentDirections.actionDashboardToCouponListFragment() | ||
) | ||
navController.navigateSafely( | ||
directions = CouponListFragmentDirections.actionCouponListFragmentToCouponDetailsFragment( | ||
couponId = event.couponId | ||
), | ||
skipThrottling = true, | ||
navOptions = navOptions { | ||
popUpTo(R.id.dashboard) | ||
anim { | ||
enter = R.anim.default_enter_anim | ||
exit = R.anim.default_exit_anim | ||
popEnter = R.anim.default_pop_enter_anim | ||
popExit = R.anim.default_pop_exit_anim | ||
} | ||
} |
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.
Instead of duplicating the couponDetailsFragment destination, I decided to just perform two navigation actions to show the details.
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 noticed that when I tap on a coupon and go back, the list isn't shown like for the reviews, but I'm back at the dashboard screen.
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.
Yes, that's intentional, since we don't have to handle the undo
behavior like in the reviews.
WidgetError( | ||
onContactSupportClicked = { /*TODO*/ }, | ||
onRetryClicked = { /*TODO*/ } | ||
onContactSupportClicked = onContactSupportClick, | ||
onRetryClicked = onRetryClick | ||
) | ||
} |
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.
Here, we will need to differentiate between the two types of errors: a generic error, or when WCAdmin is disabled, this differentiation will be added during the work on #11544
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 noticed a weird behavior when I deleted a coupon:
- I tapped on one of the coupons
- Deleted it from the coupon details menu
- When the app navigates back to the dashboard, the card shows the error screen and it's not possible to reload it
navController.navigateSafely( | ||
DashboardFragmentDirections.actionDashboardToCouponListFragment() | ||
) | ||
navController.navigateSafely( | ||
directions = CouponListFragmentDirections.actionCouponListFragmentToCouponDetailsFragment( | ||
couponId = event.couponId | ||
), | ||
skipThrottling = true, | ||
navOptions = navOptions { | ||
popUpTo(R.id.dashboard) | ||
anim { | ||
enter = R.anim.default_enter_anim | ||
exit = R.anim.default_exit_anim | ||
popEnter = R.anim.default_pop_enter_anim | ||
popExit = R.anim.default_pop_exit_anim | ||
} | ||
} |
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 noticed that when I tap on a coupon and go back, the list isn't shown like for the reviews, but I'm back at the dashboard screen.
This updates the logic to load 6 coupons instead of 3, then exclude deleted coupons, and map only the rest.
Thanks @0nko for the review, and thanks for catching the mentioned issue, it should be fixed now, please check p1716911641960219/1716911637.747699-slack-C03L1NF1EA3 for an internal discussion around the solution. |
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 addressing the issue, nice job!
Part of: #11508
Description
This PR adds the following to the coupons Card:
Testing instructions
Images/gif
Screen_recording_20240519_123512.webm
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.