Introduced UI to list domain suggestions. #8164

usamahamid · 2018-08-07T03:30:47Z

This is one of the screens required for custom domain association

This PR introduces a new screen, in which User can be search and select one of the available domain names (identified from /v1.1/domains/suggestions/ API). The navigation to this UI is not included yet, this is intended to keep this PR from affecting any working implementation.

…ons list

Introduced bare bones of an adapter, view holder and list item view components associated to the domain suggestions list. Moved Classes into a separate package for consistency.

Wired up domain suggetsions activity and fragment. Renamed layout and activity

…and clarity

wpmobilebot · 2018-08-07T03:35:34Z

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

Generated by 🚫 Danger

malinajirka · 2018-08-07T08:40:47Z

Thank you @usamahamid! Good job;).

I'm done with my first pass. I haven't tested the app though, as it's not tied to the UI yet. I've left few comments, most of which are just my preference.

Are you planning to create another PR with unit tests?

malinajirka · 2018-08-07T06:38:15Z

...ain/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsAdapter.kt

+
+    private fun onDomainSuggestionSelected(suggestion: DomainSuggestionResponse?, position: Int) {
+        selectedPosition = position
+        notifyDataSetChanged()


Can't we be more specific when notifying the observers about the change and use notifyItemChanged(int position) for example? JavaDoc of the notifyDataSetChanged() method says, it should be used as a last resort. WDYT?

Of course.
We just have to update selectedPosition and previouslySelectedPosition. I have updated the same in 1c00bdb.

malinajirka · 2018-08-07T06:40:38Z

...ain/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsAdapter.kt

+    internal fun updateSuggestionsList(items: List<DomainSuggestionResponse>) {
+        list.clear()
+        list.addAll(items)
+        notifyDataSetChanged()


Same as above. We might want to use DiffUtil. WDYT?

Since we are resetting the whole list, it might be easier and cheaper (performance wise, since we don't need the diff) to call notifyDataSetChanged().
notifyItemRangeChanged will work as well, but the size of the new suggestions list might be different, which might cause problems. What do you think?

Fair enough;)!

malinajirka · 2018-08-07T07:05:58Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
+        super.onViewCreated(view, savedInstanceState)
+
+        (activity?.application as WordPress).component()?.inject(this)


This is just my opinion, so please raise your objections if you have any. I'm a big fan of Fail ASAP principle -> the app will crash anyway, if the component is null, right? The crash will be just a bit harder to debug, because it will crash on one of the lines below. So I'd prefer to use checkNotNull(..) instead of ? to fail on our terms. Does it make sense?

malinajirka · 2018-08-07T07:24:58Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+    }
+
+    override fun onSaveInstanceState(outState: Bundle) {
+        outState.putSerializable(WordPress.SITE, viewModel.site)


Do we need to manually save the model? Doesn't it survive configuration changes in the Intent of the parent Activity? Wdyt?

Of course, since we are not updating in SiteModel here. It need not be preserved in the instanceState. 👍

malinajirka · 2018-08-07T07:37:32Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+    }
+
+    private fun setupObservers() {
+        viewModel.suggestionsLiveData.observe(this, Observer {


I prefer not having any business logic in the view. Couldn't we move this into the ViewModel so it's easily testable. Wdyt?

Understandable, I have updated the same in 56af118.

malinajirka · 2018-08-07T07:42:34Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+        })
+    }
+
+    private fun reloadSuggestions(domainSuggestions: List<DomainSuggestionResponse>) {


Again, more of a personal preference. The method is called reloadSuggestions, but it also initializes the adapter. Can't we move the initialization logic somewhere else? Maybe into the setupViews()? Wdyt?

malinajirka · 2018-08-07T08:22:21Z

...s/src/main/java/org/wordpress/android/viewmodel/domainregister/DomainSuggestionsViewModel.kt

+
+    private fun fetchSuggestions() {
+        // Disable Load more
+        suggestions = ListState.Loading(suggestions, false)


Do we need to explicitly set canLoadMore attribute to false? False is a default value, isn't it?

malinajirka · 2018-08-07T08:32:36Z

...s/src/main/java/org/wordpress/android/viewmodel/domainregister/DomainSuggestionsViewModel.kt

+        // Disable Load more
+        suggestions = ListState.Loading(suggestions, false)
+
+        val suggestDomainsPayload = SuggestDomainsPayload(searchQuery, false, false, true, 30)


We might consider creating a constant for the 30. Wdyt?

Removed SIte model from being saved in instance state, since it is not updated. Moved Recycler view adapter initialization into `setupViews`

…tifyDataSetChanged`

…he API Renamed variable.

…hould be removed before feature branch is merged to develop.

usamahamid · 2018-08-09T04:06:50Z

@malinajirka, I have updated most of PR as per your comments.
Please do check it out, when have a free moment.

I have included a new row in my site page to navigate into domain suggestions listing (for testing purposes).
I will be raising another PR with the Mockito test cases.

malinajirka

Thank you @usamahamid!

I've left one real comment (and few nitpicks:P). But it looks good, overall:). When you fix these, I believe it'll be ready for merge.

malinajirka · 2018-08-09T05:25:41Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+            suggestionsListProgress.visibility = if (it == true) View.VISIBLE else View.GONE
+        })
+        viewModel.suggestionsLiveData.observe(this, Observer {
+            if (it != null) {


I'm a just nitpicking, sorry. But the if is a business logic as well. If you just suppress all the nulls anyway, can't we just make sure the items in the LiveData are NonNull?
Note: Btw I'm not sure how the API works, but doesn't it return null when it doesn't find any results (not ideal solution, but I've seen it).

The it will never be null in here, we have defined the LiveData within the ViewModel as NonNull.
But the Observer.observe() always gives a Nullable object, since this is Kotlin, this requires a defensive check here.
We can overcome this by introducing a NonNullObserver, like mentioned in the below link.
https://proandroiddev.com/nonnull-livedata-with-kotlin-extension-26963ffd0333

What do you think?

Thank you @usamahamid for bringing this up! Definitely an interesting idea! I wouldn't delay this PR on such a minor issue. We can introduce the NonNullObserver anytime later if we see same null check pattern reoccurring.

malinajirka · 2018-08-09T05:51:17Z

...s/src/main/java/org/wordpress/android/viewmodel/domainregister/DomainSuggestionsViewModel.kt

+    // Network Request
+
+    private fun fetchSuggestions() {
+        // Disable Load more


Not sure if this comment still makes sense. Wdyt?

malinajirka · 2018-08-09T05:52:02Z

...s/src/main/java/org/wordpress/android/viewmodel/domainregister/DomainSuggestionsViewModel.kt

+            suggestions = ListState.Error(suggestions, event.error.message)
+            return
+        }
+        suggestions = ListState.Success(event.suggestions, false) // Disable load more


The false is a default value even here. We might want to remove it as well to keep it consistent. Wdyt?

malinajirka · 2018-08-09T06:11:08Z

...in/java/org/wordpress/android/ui/domainregister/suggestionslist/DomainSuggestionsFragment.kt

+            ToastUtils.showToast(activity, "Still under development.")
+        }
+        domainSearchEditText.setOnEditorActionListener { view, actionId, _ ->
+            if (actionId == EditorInfo.IME_ACTION_DONE) {


I've missed this reading the code, but testing the functionality on an emulator revealed it.

We might want to search as the user types not just when they press Done. That's why we have a handler in the ViewModel, right (the handler is used in submitSearch(..))? Also we use this approach in other parts of the app, so we should keep it consistent (eg. My Site -> Media -> Search)

…ONE` Also included minor code refactors.

usamahamid · 2018-08-09T12:33:15Z

@malinajirka, I have updated the PR to include a TextWatcher as you mentioned (which is a definite improvement). 👍
I have replied to one of your comments, Please check it out and tell me what you think. 🙂

malinajirka · 2018-08-09T13:49:54Z

Thank you for fixing the issues @usamahamid!

I'm sorry, but I have overlooked one issue, which we should fix before we merge this PR.

I've been reviewing the code and functionality, but I totally forgot to check the design. I believe some of the colors don't match the design

background, radio buttons and the choose domain button colors don't match

There are two more tiny issues, which are not in the proposed design, but we should probably try to be consistent with the SiteCreation flow.

the color of the magnifier is a bit lighter comparing to the SiteCreation flow
the progress indicator is in the center of the screen (checkout the indicator in the SiteCreation https://gyazo.com/2d0910e1608b6748e84bb15cef7e5d73)

Updated Layout design as per review comments. Additional fix to retain selection in default search screen during orientation change.

usamahamid · 2018-08-10T04:52:05Z

@malinajirka, I have updated the PR with the UI changes you requested (Seems, I missed to update the theme for the new activity).
Additionally I have fixed a minor bug where the search query is reset if it is empty, during orientation change.

The PR is ready for another pass. Please do check it out, when you have free moment.

malinajirka

Thank you @usamahamid!

After taking a closer look, I noticed that some colors still don't match. I think we should try to be consistent with the SiteCreation screen as much as possible, so the app seems polished:).

malinajirka · 2018-08-10T06:57:17Z

WordPress/src/main/res/layout/domain_suggestions_fragment.xml

+
+            <ProgressBar
+                android:id="@+id/suggestionsProgressBar"
+                android:layout_width="24dp"


We might consider extracting this into dimens.xml. Wdyt?

usamahamid · 2018-08-10T14:19:47Z

@malinajirka, I have updated the layout design to match the Site Creation layout and it is ready for another pass.
Please do check it out, when you have a free moment. 👍

malinajirka · 2018-08-10T17:45:26Z

Thank you @usamahamid! I've just done a final test and it looks awesome:)!

1a43e016c6 Merge pull request #29 from wordpress-mobile/issue/28-empty-img-tag-crash-fix a4c24373e5 Updated the release version to 1.24 a33296f4d6 Check if img source is null before checking for image smileys 1dd7406ad5 Merge pull request #27 from wordpress-mobile/combined-updates-from-wpandroid b918b23ac5 Check if announcement is available in App Settings before showing option. Updated AppLog to include announcement related tracker. 94e23e28c1 Rename build.gradle fields with invalid name syntax d0be1240ca Make sure url utils recognizes atomic image proxy as valid image url. 9bdbdff3a9 Merge branch 'feature/support-private-at-sites' of https://github.com/wordpress-mobile/WordPress-Android into feature/support-private-at-sites fcc5c99e37 Updated Photon utils to utilize new url for private AT proxy. 130195092f Gutenberg/integrate release 1.25.0 with dark mode (#11580) a25096c291 Restoring Utils lib version to 1.24 edfef1e58c Merge branch 'develop' into issue/11249-media-not-load-jetpack 60d516266e Merge pull request #11536 from wordpress-mobile/add/log-persistence 1aceca9950 Make the linter happy 18c8447fe3 Change provider visibility dac4e5ce38 Fixes to LogFileHelpersTest 76f9f75f5d Variablize recent created files test a4d0a773e7 Small fixes for LogFileCleanerTest 24ac53fabf Make the queue private 64ea6a8cdf Use correct hungarian notation for static member 1c978ca61a Refactor tests to match project refactoring 565b88903c Persist log files in a background thread 3ae86d48e4 Refactor LogEntry.toString method b900bdda83 Refactor LogFileHelpers to not use context and be a LogFileProvider 225e1db645 Upgrading lib version. 6fd47e8eb6 Using the assertThat pattern. 0948a93056 Adding connected testing. dc5c2e3cd6 Refactor LogFileWriter to use LogFileProvider instead of context 2d5d0b65c0 Refactor LogFileCleaner to not need context and use a LogFileProvider instead 84b7a8a87d Minor changes in LogFileWriter to make it more idiomatic 8cfa0950aa Merge branch 'develop' into issue/11249-media-not-load-jetpack 31a14daeb4 Bump the utils minSdkVersion to 18 f65fd78414 Add log persistence fe3960d6b7 Updating ssl logic for photon addresses. 1b4b31bf88 Merge pull request #11524 from wordpress-mobile/fix/wordpressutils-tests 6c80419806 Fix WordPressUtils tests def5be4e88 Revert "Feature/material theme and Dark Theme support (#11469)" (#11486) 16540e85f1 Feature/material theme and Dark Theme support (#11469) 0ae297bbd3 Adding ssl parameter to query for Photon https. 89725645a6 made method name more meaningful. 76343d3d6d Added util function to check if a Uri is a content one. 75790052ef Merge pull request #11072 from wordpress-mobile/issue-10843/latitude_fix_api_29 981eb82158 Merge branch 'develop' into try/media-upload-completion-processor 8799a19ec4 removes safe check due to nature of RecyclerView 7906f07645 Fixed latitude missing column issue on api 29 882a64faf3 updated gradle wrapper of utils project. b012d38c3f resolved conflicts and merged utils subtree e8aae6d282 Merge branch 'develop' into try/media-upload-completion-processor da19376391 Add MediaFile method to generate attachment page url e35a9e0cd8 Used delegate safe method for focused accessibility event listener de02499434 Merge remote-tracking branch 'origin/develop' into issue/10894-aztec-media-talkback 1812393e22 Added accessibility heading 30aa9a3132 Added a safe way to set accessibility delegates. 870518ff07 Moved method to util class that's more appropriate for it's behavior a998f6e877 added annotations to event listener method 43013a5a79 Moved behavior to Utils so that it can be replicated in other areas. 54ca204429 Removed unused imports. 373db7c13f Removed unneeded whitespace and added necessary whiteline 785da6617b WP Media Picker now announces when an image is selected. 75f0913259 "Done" button now has a content description of "Cancel" 42ec1fb96c Disabled hint announcements and applied accessibility headings 916e2b0d96 Add comment to downloadExternalMedia method 573ba7f5e8 Clean up AddMediaToEditor logic dda3a1ebb8 Merge pull request #10565 from wordpress-mobile/clean-up-after-legacy-editor d84fb1f61f Upgrade Gradle to 5.4.1, gradle plugin to 3.5.1 and fix various errors 0511354994 Remove WPEditText 19d3f4aaf0 Optimize AppLog b4401cd84f Fix simple date format concurrency issue 15ee1b27d0 Remove usages of buggy DateTimeUtils methods e9f01e5e3c Fixed loop. 47eec63356 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9815-email-verification-reminder 25d1310312 Fix AndroidX import order a21c7120d8 Fix import ordering for androidx 35b99bc73e Migrate to AndroidX d40e8773bb Merge branch 'issue/9815-create-call-to-action' into issue/9815-add-reminder-message 892eacce49 Make getPath private again ae6291c612 Use existing method instead of creating a new one in MediaUtils 10b59f8d06 Implement comments from PR f1b21d0acc Add logging for the failing cases 702881b26f A crash fix for uploaded docs cc1c0334a8 Added reminder message toast after domain has been registered 9a14564929 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions a4d8873a2e Update style config from style-config-android 1c10935b8c Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions 971cf76d96 Remove unused get snackbar duration method from accessibility utils class 28b0685ae7 Update is accessibility enabled method in accessibility utils for simplicity a418cb7dc8 Update get snackbar duration method in accessibility utils for indefinite constant e0dd4824c9 Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Android into issue/9452-domain-suggestions 506f9883ce refactored ToastUtils to be able to pass gravity as param. Also showing toast after back-dating a scheduled post on top anchor now e164cd7a61 Fixing merge conflicts from domain register branch. 586222ec51 Merge pull request #8164 from usamahamid/feature/domain-register 50de29e20a Introduced Domain Suggestion Network request in ViewModel git-subtree-dir: libs/utils git-subtree-split: 1a43e016c63bd9dc19f3e27d39b8dc1cba7c5263

usamahamid and others added 17 commits July 29, 2018 15:36

Introduced Register Domain Activity

255d9e2

Renamed package name to 'registerdomain'

4e6b01a

Introduced layout for domain picker listing page

3bb5b85

Introduced empty and progress states layout items for domain suggesti…

c383b1e

…ons list

Introduced domain picker fragment and view model

0dd9c91

Introduced Domain Suggestion Network request in ViewModel

253b1b9

Introduced Domain suggestions recycler view components

4ee1284

Introduced bare bones of an adapter, view holder and list item view components associated to the domain suggestions list. Moved Classes into a separate package for consistency.

Introduced selection event flow for selecting a domain suggestion

98b77f5

Wired up domain suggetsions activity and fragment

cb36be7

Wired up domain suggetsions activity and fragment. Renamed layout and activity

Wired Up OnSelection listener for list item click event.

51bb57f

Introduced retaining item selection on orientation change.

c982b28

Updated Layout to utilize the dimens, instead of hard coding them.

19eb8d4

Introduced initial suggestions from site name.

0a33480

Removed Activity launcher, introduced for testing.

0422136

Rename 'registerdomain' package into 'domainregister' for consitency …

fa2b9b1

…and clarity

Don't include Wordpress.com sites on domain suggestions list

8ecacbe

Show 30 domian suggestions to be consistent with 'Wordpress.com'

ed6ef4a

Fixed ktlint.

76e87d2

malinajirka self-assigned this Aug 7, 2018

malinajirka added the Domain Search label Aug 7, 2018

malinajirka added this to the 10.8 milestone Aug 7, 2018

malinajirka reviewed Aug 7, 2018

View reviewed changes

usamahamid changed the base branch from develop to feature/domain-register August 7, 2018 08:49

usamahamid added 5 commits August 9, 2018 05:51

Refactors and minor fixes

20b679b

Removed SIte model from being saved in instance state, since it is not updated. Moved Recycler view adapter initialization into `setupViews`

Updated adapter to utlize the notifyItemChanged instead ofusing `no…

1c00bdb

…tifyDataSetChanged`

Introduced constant to hold the count of suggestions requested from t…

e7e1315

…he API Renamed variable.

Introduced row in MySites Screen, this is only testing purposes and s…

f98bc22

…hould be removed before feature branch is merged to develop.

Moved business logic into ViewModel

56af118

Fixed ktlint

2941df4

malinajirka reviewed Aug 9, 2018

View reviewed changes

Utilize TextWatcher instead of waiting for keyboard's `IME_ACTION_D…

91640fd

…ONE` Also included minor code refactors.

usamahamid added 2 commits August 10, 2018 10:09

Introduced Domain Registration theme.

c1eacd8

Updated Layout design as per review comments. Additional fix to retain selection in default search screen during orientation change.

Fixed typo.

0b68172

malinajirka reviewed Aug 10, 2018

View reviewed changes

Updated Text, hint and Icon colors to be similar to Site Creation

37e6ff5

malinajirka closed this Aug 10, 2018

malinajirka reopened this Aug 10, 2018

malinajirka merged commit c7bd8ee into wordpress-mobile:feature/domain-register Aug 10, 2018

Introduced UI to list domain suggestions. #8164

Introduced UI to list domain suggestions. #8164

Uh oh!

Conversation

usamahamid commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamahamid Aug 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamahamid Aug 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamahamid commented Aug 9, 2018

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamahamid Aug 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usamahamid commented Aug 9, 2018

Uh oh!

malinajirka commented Aug 9, 2018

Uh oh!

usamahamid commented Aug 10, 2018

Uh oh!

malinajirka 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!

Choose a reason for hiding this comment

Uh oh!

usamahamid commented Aug 10, 2018

Uh oh!

malinajirka commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

usamahamid commented Aug 7, 2018 •

edited

Loading

wpmobilebot commented Aug 7, 2018 •

edited

Loading

usamahamid Aug 9, 2018 •

edited

Loading

usamahamid Aug 9, 2018 •

edited

Loading

usamahamid Aug 9, 2018 •

edited

Loading

malinajirka left a comment •

edited

Loading