Update free domain suggestions UI #17998

ovitrif · 2023-02-21T17:07:47Z

Resolves #17899

This PR:

⊕ Adds the new UI for domain suggestions using Compose Update free domain suggestions UI #17899
⊕ Refactors view logic for domain suggestions:
- ⊕ Migrates the domain suggestion view-holder to view binding
- ⊕ Refactors the UI state of domain list items to separate the old vs new one
- ⊖ Removes the repetitive Domain prefixes from ui state class names when it can be deduced
- ⊜ Converts single line vm methods from block to expression body

Note
To easily review the many changes please go commit by commit.

To Test

Prerequisites:

◐ Toggle the Site Creation Domain Purchasing feature (SiteCreationDomainPurchasingFeatureConfig)

Go to Me → App Settings → Debug settings
Scroll to the Features in development section
Tap on SiteCreationDomainPurchasingFeatureConfig
Tap RESTART THE APP button

🅰️ Control Variation

Open the Jetpack app and login
◐ Toggle the Site Creation Domain Purchasing feature flag OFF (should be the case)
Start the Site Creation flow: Open Site Picker → ➕ → Create WordPress.com site
Continue the flow till Choose a domain screen
Verify the expected results functionality is maintained by the refactoring:
1. Enter test in the search field
2. Expect the first result to be the unavailable test.wordpress.com followed by available one(s)
3. Tap on the ✗ button to clear the search query
4. Expect The results to be replaced by the placeholder illustration
5. Enter tested in the search field
6. Expect all results to be available and selectable
Verify the expected error functionality keeps working:
1. Tap on the ✗ button to clear the search query
2. Turn off internet connectivity on your testing device
3. Enter testet in the search field
4. Expect The result to be There is no network available
5. Turn on internet connectivity
6. Tap the error item (e.g. on Retry)
7. Expect the results to be updated
8. Replace the t in the search field with %
9. Expect the error message for invalid characters

🅱️ Treatment Variation

◐ Toggle the Site Creation Domain Purchasing feature flag ON
Start the Site Creation flow: Open Site Picker → ➕ → Create WordPress.com site
Continue the flow till Choose a domain screen
Verify free and paid domains are suggested
Tap on a free domain
Verify the Create Site button is displayed
Tap on a paid domain
Verify the Create Site button is hidden

Screenshots

🅰️ Control	🅱️ Treatment
Before	After

Regression Notes

Potential unintended areas of impact
Site creation → Domains when the experimental feature is disabled
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
Updated unit tests in SiteCreationDomainsViewModelTest. I don't think more tests are needed since most of the changes are in the view layer and the improved ui state strong typing is a solid way to guard against regressions when it comes to view-model.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

wpmobilebot · 2023-02-21T17:24:03Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17998-ab46b8f.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`ab46b8f`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2023-02-21T17:24:49Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17998-ab46b8f.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`ab46b8f`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

ParaskP7 · 2023-02-22T10:23:19Z

👋 @ovitrif !

I am seeing being included in this feature related PR review, alongside other feature engineers, was that on purpose, is there is anything core specific that you would like me to review here? 🤔

...ss/src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsAdapter.kt

ovitrif · 2023-02-22T10:36:44Z

👋 @ovitrif !

I am seeing being included in this feature related PR review, alongside other feature engineers, was that on purpose, is there is anything core specific that you would like me to review here? 🤔

👋 Petros, you're uber fast 🚀 👀 🏆
Please see the comment I added afterwards 🙏 !

thomashorta

Nice job! I added a few comments (mostly questions and minor suggestions).

Also, while testing it I noticed a couple of things and just wanted to confirm with you that this is the intender behavior right now.

When following this step: "Replace the t in the search field with %" both in the Control and Treatment Variations, I found that I only get the error if there are ONLY invalid characters, is that correct?
In the Treatment Variation, selecting an item doesn't make it "selected" (highlighted or something visually different), is that on purpose for now?

Screenshots for question 1:

Valid + Invalid Chars	Only Invalid Chars

.../src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainViewHolder.kt

...ss/src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsAdapter.kt

.../src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsViewModel.kt

WordPress/src/main/java/org/wordpress/android/ui/sitecreation/domains/compose/DomainItem.kt

.../test/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsViewModelTest.kt

.../src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsViewModel.kt

thomashorta

🛑 Uh-oh, I was testing it a bit more and I found a crash 😬

First, I found that the animations get janky when tapping a free domain and that the keyboard is shown again even though I just hid it.

Then, I found out that when tapping 2 different free domains alternately quickly, the app crashes.

It seems it happens because this changes the selectedDomain here, causing an UI state update, which triggers this observable here and causes the whole UI to be rebuilt.

This causes the recycler view holder to call onBind again, but if done too quickly it seems to be causing a call to a LifecycleOwner that no longer exists (or something like that) when calling this line.

Crash video (OnePlus 5T on Android 10) ↘️

pr-17998-quick-tap-crash.mp4

See the crash logs below ↘️

FATAL EXCEPTION: main
Process: com.jetpack.android.prealpha, PID: 15414
java.lang.IllegalStateException: View tree for androidx.compose.ui.platform.ComposeView{9a4a4be V.E...... .......D 0,0-1080,142 #7f0a019b app:id/compose_view} has no ViewTreeLifecycleOwner
	at androidx.compose.ui.platform.ViewCompositionStrategy$DisposeOnViewTreeLifecycleDestroyed.installFor(ViewCompositionStrategy.android.kt:109)
	at androidx.compose.ui.platform.AbstractComposeView.setViewCompositionStrategy(ComposeView.android.kt:140)
	at org.wordpress.android.ui.sitecreation.domains.SiteCreationDomainViewHolder$DomainComposeItemViewHolder.onBind(SiteCreationDomainViewHolder.kt:74)
	at org.wordpress.android.ui.sitecreation.domains.SiteCreationDomainsAdapter.onBindViewHolder(SiteCreationDomainsAdapter.kt:40)
	at org.wordpress.android.ui.sitecreation.domains.SiteCreationDomainsAdapter.onBindViewHolder(SiteCreationDomainsAdapter.kt:19)
	at androidx.recyclerview.widget.RecyclerView$Adapter.onBindViewHolder(RecyclerView.java:7254)
	at androidx.recyclerview.widget.RecyclerView$Adapter.bindViewHolder(RecyclerView.java:7337)
	at androidx.recyclerview.widget.RecyclerView$Recycler.tryBindViewHolderByDeadline(RecyclerView.java:6194)
	at androidx.recyclerview.widget.RecyclerView$Recycler.tryGetViewHolderForPositionByDeadline(RecyclerView.java:6460)
	at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6300)
	at androidx.recyclerview.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:6296)
	at androidx.recyclerview.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:2330)
	at androidx.recyclerview.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1631)
	at androidx.recyclerview.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1591)
	at androidx.recyclerview.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:668)
	at androidx.recyclerview.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:4309)
	at androidx.recyclerview.widget.RecyclerView.dispatchLayout(RecyclerView.java:4012)
	at androidx.recyclerview.widget.RecyclerView.onLayout(RecyclerView.java:4578)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at androidx.constraintlayout.widget.ConstraintLayout.onLayout(ConstraintLayout.java:1855)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at androidx.core.widget.NestedScrollView.onLayout(NestedScrollView.java:1983)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1103)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at com.google.android.material.appbar.HeaderScrollingViewBehavior.layoutChild(HeaderScrollingViewBehavior.java:148)
	at com.google.android.material.appbar.ViewOffsetBehavior.onLayoutChild(ViewOffsetBehavior.java:43)
	at com.google.android.material.appbar.AppBarLayout$ScrollingViewBehavior.onLayoutChild(AppBarLayout.java:2119)
	at androidx.coordinatorlayout.widget.CoordinatorLayout.onLayout(CoordinatorLayout.java:918)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.RelativeLayout.onLayout(RelativeLayout.java:1103)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.LinearLayout.setChildFrame(LinearLayout.java:1829)
	at android.widget.LinearLayout.layoutVertical(LinearLayout.java:1673)
	at android.widget.LinearLayout.onLayout(LinearLayout.java:1582)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.widget.FrameLayout.layoutChildren(FrameLayout.java:332)
	at android.widget.FrameLayout.onLayout(FrameLayout.java:270)
	at com.android.internal.policy.DecorView.onLayout(DecorView.java:786)
	at android.view.View.layout(View.java:22085)
	at android.view.ViewGroup.layout(ViewGroup.java:6290)
	at android.view.ViewRootImpl.performLayout(ViewRootImpl.java:3363)
	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2834)
	at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1947)
	at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8048)
	at android.view.Choreographer$CallbackRecord.run(Choreographer.java:1163)
	at android.view.Choreographer.doCallbacks(Choreographer.java:986)
	at android.view.Choreographer.doFrame(Choreographer.java:902)
	at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:1148)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loop(Looper.java:214)
	at android.app.ActivityThread.main(ActivityThread.java:7697)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:516)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:950)

ovitrif · 2023-02-23T09:14:15Z

Thanks for your great review @thomashorta 🙇

When following this step: "Replace the t in the search field with %" both in the Control and Treatment Variations, I found that I only get the error if there are ONLY invalid characters, is that correct?

Yep that's the expected and existing behavior on trunk or prod. I guess the backend does some sort of sanitization and it return matches for that sanitized query when it's not empty.

📮 Postman screenshots when querying the /domains/suggestions/ GET endpoint:

Results for query: t%

Error for query: %x

In the Treatment Variation, selecting an item doesn't make it "selected" (highlighted or something visually different), is that on purpose for now?

True, that's because we were still deciding on the new UI, and this task was the initial plan: #17901 where we would have shown a modal instead of any a visual indication on the selected item 😉 .
This PR is only concerned with the specs from the related task: #17899 👍

ovitrif · 2023-02-23T09:42:17Z

🛑 Uh-oh, I was testing it a bit more and I found a crash 😬

Thanks a lot for finding this bug @thomashorta before we merge the PR. It almost slipped through and we could have missed it 🤦 . This really reminds me how important it is to a little more exploratory testing in every PR and continue to dedicate the last week of a project to testing, a ritual we started last year in our team.

I was able to repro it and proceeded to address the issue in this commit: 039ea86. It doesn't reproduce for me anymore 🤞

Thanks for investigating the call stack leading to it.
P.S. Please don't feel like you have to do this on my PRs. It's something I have to own and I'm totally ok if you just say: "Hey Ovi, look, your PR might have introduced this bug that I can repro with these steps: [..]".

ovitrif · 2023-02-23T10:30:48Z

I found that the animations get janky

@thomashorta Can you make a recording of this or explain how I could repro this?
I couldn't repro it myself.

Thanks in advance 🙏

the keyboard is shown again even though I just hid it

Another great discovery @thomashorta 🥇 . This seems to be a bug in production as well, I filed issue #18004 and added it on the project board.

I won't fix it it this PR since it's already an existing issue and this PR is starting to be concerned with too many things 😅 .

ovitrif · 2023-02-23T11:43:16Z

👋 @antonis (and @thomashorta) This PR is ready for another pass.

The crash has been resolved in 039ea86. I don't see any unaddressed issues except #18004 which is in production as well so we skip it on purpose 🤞

thomashorta · 2023-02-23T12:28:07Z

@thomashorta Can you make a recording of this or explain how I could repro this? I couldn't repro it myself.

Thanks in advance 🙏

The original crash video I posted shows the animation issue, but it is subtle.

The animation I'm referring to is the ripple effect on item clicks when tapping a free domain that is not the currently selected one. It is very subtle so maybe not worth taking a look now, especially because I think it might be related to using Compose inside a RecyclerView and you added some TODOs regarding updating Compose and RecyclerView versions for better support.

Here's a video that makes the issue clearer by tapping twice on the same item. Notice how the first animation stops abruptly and the second one (which doesn't trigger a UI update) has a smoother fade-out. I also slowed down the animations using Developer Settings so it's easier to see, in the second part of the video.

pr-17998-ripple-animation-update.mp4

But again, I'm fine with keeping it this way for now, as it is probably related to the RecyclerView + Compose interaction with the lib versions we're using.

thomashorta

LGTM and the crash doesn't happen anymore. Thanks!

antonis

Thank you for the update @ovitrif and for the extensive review @thomashorta 🙇

The #17998 (comment) has been resolved in 039ea86. I don't see any unaddressed issues except #18004 which is in production as well so we skip it on purpose 🤞

I've retested with a debug build on my Pixel 5 / Android 13 and wasn't able to reproduce the crash or any other new issue. The code after the updates also LGTM 🎉

ovitrif · 2023-02-23T13:46:45Z

Here's a video that makes the issue clearer by tapping twice on the same item. Notice how the first animation stops abruptly and the second one (which doesn't trigger a UI update) has a smoother fade-out. I also slowed down the animations using Developer Settings so it's easier to see, in the second part of the video.

Thanks @thomashorta. It's rather inconsistent than "janky" IMHO.
Yeah, very possible it's related to using a compose view inside a recycler view 🤷 .
Doesn't feel like a blocker to me neither 👍

ovitrif · 2023-02-23T13:48:15Z

Gonna merge this once the PR of the base branch can be merged to trunk, thanks everyone for the feedback 🙇

ovitrif changed the base branch from trunk to issue/17976-fetch-free-and-paid-domains February 21, 2023 17:08

ovitrif added [Type] Task Site Creation Jetpack App Domains labels Feb 21, 2023

ovitrif mentioned this pull request Feb 21, 2023

Add UI for paid domains #17993

Merged

3 tasks

ovitrif added this to the 21.9 milestone Feb 22, 2023

ovitrif marked this pull request as ready for review February 22, 2023 10:18

ovitrif requested review from ParaskP7, antonis and thomashorta February 22, 2023 10:18

ovitrif commented Feb 22, 2023

View reviewed changes

...ss/src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsAdapter.kt Show resolved Hide resolved

ovitrif mentioned this pull request Feb 22, 2023

Dependency Updates - Main Batch - AndroidX Compose/Kotlin #17563

Closed

9 tasks

thomashorta reviewed Feb 22, 2023

View reviewed changes

antonis reviewed Feb 22, 2023

View reviewed changes

.../src/main/java/org/wordpress/android/ui/sitecreation/domains/SiteCreationDomainsViewModel.kt Outdated Show resolved Hide resolved

thomashorta suggested changes Feb 22, 2023

View reviewed changes

ovitrif requested a review from antonis February 22, 2023 20:50

thomashorta approved these changes Feb 23, 2023

View reviewed changes

antonis approved these changes Feb 23, 2023

View reviewed changes

ovitrif added 26 commits February 23, 2023 18:07

Refactor: Add onClick to constructor of available domain ui state

3b68c18

Refactor: Move all logic in domain suggestion item view-holder to onBind

8e8b7bc

Refactor: Move all logic in domain suggestion error viewHolder to onBind

12fc2d9

Refactor: Remove redundant requireNotNull from domain error viewHolder

938aee4

Refactor: Pass specific uiState type to domain view holders

df50e23

Refactor: Cleanup and tidy logic in domain view holders

ec483c4

WIP: Add compose view for new domains ui

236d18f

Refactor: Remove deducible qualifying prefixes from ui-state models

627fcda

Refactor: Separate old item ui state from the new one by sealed class

24bb248

Refactor: Remove temporary cost added to domain name

739383e

Add and display raw domain cost

32c7d84

Implement onClick for free domains

c3cbdbd

Hide old unavailability item in new list ui

40b381b

Fix checkstyle errors

7c83d48

Suppress ReturnCount check to fix detekt issues

1deb81a

Add divider and gray cost text color

9c5e3f2

Fix unit tests for old domains ui

5f258e2

Revert unwanted change

5af7052

Remove unnecessary forbidden comment suppression

e73ffae

Refactor: Rename query update functions

47940d1

Refactor: Rename view-holder classes to separate the old vs new

efb445c

Refactor: Put parameters on one line in view-holders

92303c1

Refactor: Rename ERROR_FETCH to ERROR_V1

596d58e

Refactor: Cleanup empty list message logic to get rid of the comment

f4f37dd

Refactor: Remove braces from if statement in DomainsDiffUtils

047407e

Fix compose crash when selecting items quickly by moving config to init

ab46b8f

ovitrif force-pushed the issue/17899-free-domain-suggestions-ui branch from 039ea86 to ab46b8f Compare February 23, 2023 17:09

Base automatically changed from issue/17976-fetch-free-and-paid-domains to trunk February 23, 2023 17:29

ovitrif merged commit 782e6fa into trunk Feb 23, 2023

ovitrif deleted the issue/17899-free-domain-suggestions-ui branch February 23, 2023 17:36

Update free domain suggestions UI #17998

Update free domain suggestions UI #17998

Uh oh!

Conversation

ovitrif commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To Test

Prerequisites:

🅰️ Control Variation

🅱️ Treatment Variation

Screenshots

Regression Notes

Uh oh!

wpmobilebot commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ParaskP7 commented Feb 22, 2023

Uh oh!

Uh oh!

ovitrif commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomashorta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomashorta left a comment • edited by ovitrif Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovitrif commented Feb 23, 2023

Uh oh!

ovitrif commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovitrif commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ovitrif commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomashorta commented Feb 23, 2023

Uh oh!

thomashorta left a comment

Choose a reason for hiding this comment

Uh oh!

antonis left a comment

Choose a reason for hiding this comment

Uh oh!

ovitrif commented Feb 23, 2023

Uh oh!

ovitrif commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ovitrif commented Feb 21, 2023 •

edited

Loading

wpmobilebot commented Feb 21, 2023 •

edited

Loading

wpmobilebot commented Feb 21, 2023 •

edited

Loading

ovitrif commented Feb 22, 2023 •

edited

Loading

thomashorta left a comment •

edited

Loading

thomashorta left a comment •

edited by ovitrif

Loading

ovitrif commented Feb 23, 2023 •

edited

Loading

ovitrif commented Feb 23, 2023 •

edited

Loading

ovitrif commented Feb 23, 2023 •

edited

Loading

ovitrif commented Feb 23, 2023 •

edited

Loading