-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Domain transfer dashboard card #18852
Conversation
Create card layout Create ViewHolder Create and update card builder Add preferences for hiding card Add tracks Add navigation to launch transfer page Update MySiteViewModel.kt Update MySiteTabFragment.kt
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
Use ActivityNavigator in lieu of Activity Launcher
Updated content to latest copy
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.
Hey @ravishanker
I have tested this PR according to the instructions in the PR description. Everything looks good to me. I have left a ❓ Question, 💡 Suggestion. They are not blocking, so approving but not merging so that you can address them in this PR if necessary. If you think, there is no changes necessary/can be taken up in another PR, feel free to merge 👍🏼
context: Context, | ||
url: String | ||
) { | ||
WPWebViewActivity.openUrlByUsingGlobalWPCOMCredentials(context, url) |
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.
👏🏼 [praise] : Thanks for using this class
WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt
Outdated
Show resolved
Hide resolved
...rdpress/android/ui/mysite/cards/dashboard/domaintransfer/DashboardCardDomainTransferUtils.kt
Outdated
Show resolved
Hide resolved
Hey @ravishanker I was getting some build errors, mostly due to the migration to the latest AGP. Hence I updated this branch with |
Generated by 🚫 dangerJS |
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.
Hey @ravishanker
Reviewed it again, everything looks good to me 👍🏼 . Approved, enabling automerge
Adds dashboard card for domain transfer
Fixes: See
To test:
Launch Jetpack app
Verify
domains transfer card appears as shown above on dashboardTap
on the Learn more or anywhere on the cardVerify
it launches domain transfer pageTap
on overflow (three dots) menu on the cardVerify
it pops up menu with Hide this optionTap
on Hide thisVerify
, the card gets hiddenCheck for the following tracks in AS
🔵 Tracked: dashboard_card_domain_transfer_shown
🔵 Tracked: dashboard_card_domain_transfer_more_menu_tapped
🔵 Tracked: dashboard_card_domain_transfer_hidden
Regression Notes
Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)
Unit tests and manual tests
What automated tests I added (or what prevented me from doing so)
Updated and added unit tests
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: