Improve Search Result page #85

coryroy · 2017-04-25T21:57:48Z

What

Add Baseball style project cards to search
Add clearer reference tags
Refactor into new style with ViewModels inside ViewHolders

…ethod

…SS if just font size an colour. :)

…or as a substitutable in the project stats string

* Add new event names and missing (clear) event * Fix broken unit tests. * Put back legacy events * added test

…droid-oss into update_search_result_list

… method to insert sections

coryroy · 2017-04-28T19:17:19Z

Updated layout to top align with padding

…droid-oss into update_search_result_list # Conflicts: # app/src/main/assets/json/server-config.json

mbrandonw

woot! think we're done with this one! great job!

mbrandonw · 2017-04-26T20:38:06Z

app/src/main/java/com/kickstarter/ui/activities/SearchActivity.java

@@ -4,6 +4,7 @@
 import android.os.Bundle;
 import android.support.annotation.NonNull;
 import android.support.annotation.Nullable;
+import android.support.v4.util.Pair;


what's this jazz?

mbrandonw · 2017-04-26T20:38:22Z

app/src/main/java/com/kickstarter/ui/activities/SearchActivity.java

    final Intent intent = new Intent(this, ProjectActivity.class)
      .putExtra(IntentKey.PROJECT, project)
-      .putExtra(IntentKey.REF_TAG, RefTag.search());
+      .putExtra(IntentKey.REF_TAG, didSearch() ? refTags.first : refTags.second);


dont wanna put logic here

mbrandonw · 2017-04-26T20:38:47Z

app/src/main/java/com/kickstarter/ui/adapters/SearchAdapter.java

@@ -27,29 +27,41 @@ public void loadPopularProjects(final @NonNull List<Project> newProjects) {
    if (newProjects.size() == 0) {
      addSection(Collections.emptyList());
    } else {
-      addSection(Collections.singletonList(null));
+      addSection(getFeatureProject(newProjects));


refactor this

mbrandonw · 2017-04-26T20:45:41Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

+    this.viewModel.outputs.projectName()
+      .compose(bindToLifecycle())
+      .compose(observeForUI())
+      .subscribe(this::setProjectName);


can do .subscribe(this.projectNameTextView::setText);

mbrandonw · 2017-04-27T13:41:43Z

app/src/test/java/com/kickstarter/viewmodels/SearchViewModelTest.java

+    scheduler.advanceTimeBy(300, TimeUnit.MILLISECONDS);
+    final TestSubscriber<RefTag> projectClicked = new TestSubscriber<>();
+    viewModel.outputs.searchProjects().map(sp -> sp.get(0)).subscribe(viewModel.inputs::tappedProject);
+    viewModel.outputs.goToProject().map(p -> p.second).subscribe(projectClicked);


we need to attach test subscribers immediately and we shouldn't be hitting vm inputs like that

mbrandonw · 2017-04-27T13:55:14Z

app/src/main/java/com/kickstarter/viewmodels/SearchViewModel.java

@@ -38,6 +42,8 @@

    /** Emits list of projects matching criteria. */
    Observable<List<Project>> searchProjects();
+
+    Observable<Pair<Project, RefTag>> goToProject();


mbrandonw · 2017-04-27T13:55:17Z

app/src/main/java/com/kickstarter/viewmodels/SearchViewModel.java

@@ -30,6 +32,8 @@

    /** Call when text changes in search box. */
    void search(final @NonNull String s);
+
+    void tappedProject(final @NonNull Project project);


luoser

a few changes and questions

luoser · 2017-05-01T21:33:36Z

app/src/main/java/com/kickstarter/ui/viewholders/PopularSearchTitleViewHolder.java


-  protected @Bind(R.id.search_term_text_view) TextView termTextView;
-  protected @Bind(R.id.search_term_view) LinearLayout layout;
+  @Bind(R.id.search_term_text_view) TextView termTextView;


can name this searchTermTextView to match the id

luoser · 2017-05-01T21:35:16Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

+  @Bind(R.id.project_name_text_view) TextView projectNameTextView;
+  @Bind(R.id.project_image_view) ImageView projectImageView;
+  @Bind(R.id.project_stats_text_view_pct_complete_data) TextView projectStatsPctCompleteDataTextView;
+  @Bind(R.id.project_stats_text_view_pct_complete_string) TextView projectStatsPctCompleteStringTextView;


just curious as to what Pct stands for? it's not a very intuitive abbreviation for me

Percent. What would you prefer?

I've changed it to percent. We can shorten it to something else if you prefer.

luoser · 2017-05-01T21:35:30Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java


-    ((KSApplication) view.getContext().getApplicationContext()).component().inject(this);


luoser · 2017-05-01T21:36:05Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

+    this.viewModel.inputs.configureWith(configData);
+  }
+
+  void setProjectImage(final String imageUrl) {


can mark this and the following method as private and the param as @NonNull

luoser · 2017-05-01T21:36:41Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

-    } else {
-      projectImageView.setVisibility(View.INVISIBLE);
-    }
+  void setProjectStats(final Pair<Integer, Integer> stats) {


same here, private method, nonnull param

luoser · 2017-05-01T21:46:22Z

app/src/main/res/layout/search_popular_title_view.xml

+    android:textColor="@color/gray_black"
+    tools:text="@string/search_most_popular" />
+
+</LinearLayout>


luoser · 2017-05-01T21:46:35Z

app/src/main/res/values/dimens.xml

@@ -69,6 +69,8 @@
  <dimen name="comment_body_padding_x">@dimen/grid_7</dimen>
  <dimen name="feed_padding_y">@dimen/grid_5_half</dimen>
  <dimen name="feed_padding_x">@dimen/grid_2</dimen>
+  <dimen name="search_padding_y">@dimen/grid_new_2</dimen>
+  <dimen name="search_padding_x">@dimen/grid_new_2</dimen>


luoser · 2017-05-01T21:46:56Z

app/src/test/java/com/kickstarter/viewmodels/ProjectSearchResultHolderViewModelTest.java

+  private final TestSubscriber<String> projectName = new TestSubscriber<>();
+  private final TestSubscriber<Pair<Integer, Integer>> projectStats = new TestSubscriber<>();
+
+  @Before


do we need this annotation?

In order to have it run before each test we do. I have now made it override parent setUp and passing the resulting environment

luoser · 2017-05-01T21:47:29Z

app/src/test/java/com/kickstarter/viewmodels/ProjectSearchResultHolderViewModelTest.java

+
+  @Before
+  public void setUpEnvironment() {
+    this.vm = new ProjectSearchResultHolderViewModel.ViewModel(environment());


i know we have access to the environment within our test files, but it's good practice to pass in the environment in the method in case we need to modify it within each test in the future

luoser · 2017-05-01T21:48:19Z

app/src/test/java/com/kickstarter/viewmodels/SearchViewModelTest.java

+      KoalaEvent.CLEARED_SEARCH_TERM);
+  }
+
+  void testSearchPagination() {


missing @Test annotation, and should be marked public

mbrandonw · 2017-05-02T14:59:10Z

app/src/main/java/com/kickstarter/viewmodels/SearchViewModel.java

-        this.popularProjects.asObservable(),
-        this.searchProjects.asObservable()
+        this.popularProjects,
+        this.searchProjects


oh good catch. I did that. didn't know you could merge subjects, thought you had to get into the observable under the hood.

…ther test

… sensible project names

luoser

just some more small code style things i've mentioned before, but looks good!

luoser · 2017-05-02T21:17:41Z

app/src/main/java/com/kickstarter/ui/viewholders/KSViewHolder.java

@@ -34,7 +34,7 @@ public KSViewHolder(final @NonNull View view) {
   */
  @Override
  public void onClick(final @NonNull View view) {
-    Timber.d("Default KSViewHolder onClick event");


this shouldn't change since it's describing the default value, i.e. not every VH onClick will be a project click

luoser · 2017-05-02T21:18:10Z

app/src/main/java/com/kickstarter/ui/viewholders/PopularSearchTitleViewHolder.java


 import butterknife.Bind;
 import butterknife.BindString;
 import butterknife.ButterKnife;

-public final class SearchTermViewHolder extends KSViewHolder {
-  private DiscoveryParams params;
+public class PopularSearchTitleViewHolder extends KSViewHolder {


this class should be final, in general make sure our classes are final

luoser · 2017-05-02T21:18:42Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

+    this.viewModel.inputs.configureWith(configData);
+  }
+
+  private void setProjectImage(@NonNull final String imageUrl) {


final @NonNull

luoser · 2017-05-02T21:19:19Z

app/src/main/java/com/kickstarter/ui/viewholders/ProjectSearchResultViewHolder.java

-    } else {
-      projectImageView.setVisibility(View.INVISIBLE);
-    }
+  private void setProjectStats(@NonNull final Pair<Integer, Integer> stats) {


same here final @NonNull

luoser · 2017-05-02T21:20:40Z

app/src/main/java/com/kickstarter/viewmodels/SearchViewModel.java

    private final PublishSubject<String> search = PublishSubject.create();

    private final BehaviorSubject<List<Project>> popularProjects = BehaviorSubject.create();
    private final BehaviorSubject<List<Project>> searchProjects = BehaviorSubject.create();
+    private final BehaviorSubject<Pair<Project, RefTag>> goToProject = BehaviorSubject.create();


can alphabetize this

luoser · 2017-05-02T21:20:56Z

app/src/main/java/com/kickstarter/viewmodels/SearchViewModel.java

    @Override public void search(final @NonNull String s) {
      this.search.onNext(s);
    }

+    @Override public Observable<Pair<Project, RefTag>> goToProject() {


@NonNull observable for all these outputs

luoser · 2017-05-02T21:21:31Z

app/src/main/res/layout/search_popular_title_view.xml

+    android:textColor="@color/gray_black"
+    tools:text="@string/Popular_Projects" />
+
+</LinearLayout>


luoser · 2017-05-02T21:22:34Z

app/src/test/java/com/kickstarter/ui/adapters/SearchAdapterTest.java

+import java.util.Collections;
+import java.util.List;
+
+public class SearchAdapterTest extends KSRobolectricTestCase implements SearchAdapter.Delegate {


final, but also 🎉 our first adapter test!!

luoser · 2017-05-02T21:24:27Z

app/src/test/java/com/kickstarter/viewmodels/SearchViewModelTest.java

+    scheduler.advanceTimeBy(300, TimeUnit.MILLISECONDS);
+    viewModel.inputs.projectClicked(projects.get(0));
+
+    goToRefTag.assertValues(RefTag.searchFeatured());


so great to have tests for this kind of business logic

luoser · 2017-05-02T21:24:56Z

app/src/test/java/com/kickstarter/viewmodels/SearchViewModelTest.java

+    scheduler.advanceTimeBy(300, TimeUnit.MILLISECONDS);
+
+    projectList.assertValueCount(2);
+


extra newline

undeadly added 14 commits April 20, 2017 16:51

Show metadata in search list (no styling or updated design yet)

80de1fd

Beginning to add in the style to the search list view

6f34f23

Change some variable names and breakout spannable creation into new m…

7d00093

…ethod

Remove deprecated method

8fa4f09

change some variable names for clarity

827d00b

Now showing featured item

b8ed345

Add new ref tags

d38daee

Remove extra dimens and colour

08f869b

Merge remote-tracking branch 'oss/master' into update_search_result_list

9538722

Switched to html textview rather than spannable for better i18n. No C…

050a617

…SS if just font size an colour. :)

Now using a viewmodel for view holder, including test. Also added col…

411ea6d

…or as a substitutable in the project stats string

Fix some formatting issues

218ba33

Update the layout and styling to better match mockup and iOS for search

4d76ba4

Fix whitespace issues

df6aea7

coryroy requested a review from luoser April 25, 2017 21:57

undeadly and others added 2 commits April 25, 2017 16:09

Changed codestyle to keep the milkrun happy

8ec1267

Merge remote-tracking branch 'oss/master' into update_search_result_list

3301960

coryroy requested a review from mbrandonw April 26, 2017 16:21

mbrandonw and others added 12 commits April 26, 2017 17:08

Merge remote-tracking branch 'oss/master' into update_search_result_list

675d718

bringing some logic into the vm

2c39cfe

Add tests for new reftags and fix lint issue

2a9289f

Add new event names and missing (clear) event for Search Page (#86)

00f5089

* Add new event names and missing (clear) event * Fix broken unit tests. * Put back legacy events * added test

test clean up

4bf932b

Merge remote-tracking branch 'oss/master' into update_search_result_list

0268a4b

Refactor tests to better match testing style and provide better coverage

f771fc0

Fix lint issues

8299607

Merge branch 'update_search_result_list' of github.com:kickstarter/an…

83388c8

…droid-oss into update_search_result_list

cleaned up search data source

86fb99c

Separate Popular Projects to it's own view. Fix adapter to use proper…

409c6f6

… method to insert sections

Rename a couple files and move method to constructor (from bindData)

cdeb58a

undeadly added 2 commits April 28, 2017 10:42

Fix layout and shine up the code a bit.

d301911

Align card info to top, but with padding

c020e7d

undeadly added 6 commits April 28, 2017 13:47

Add days to the "to go" text

2d0d656

Create new dimension rather than modifying one used elsewhere

cb17acd

Rename method to sound less iossey

4e8256b

Merge branch 'master' into update_search_result_list

87f5aa9

Update strings to get translations in

dcfe1de

Merge branch 'update_search_result_list' of github.com:kickstarter/an…

b9f18f3

…droid-oss into update_search_result_list # Conflicts: # app/src/main/assets/json/server-config.json

mbrandonw approved these changes May 1, 2017

View reviewed changes

Merge branch 'master' into update_search_result_list

f36f029

luoser suggested changes May 1, 2017

View reviewed changes

undeadly added 3 commits May 1, 2017 18:19

Code style improvements

ad88abd

Code style improvements

b1074e8

Remove extra 'asObservable'

a2e8691

mbrandonw reviewed May 2, 2017

View reviewed changes

undeadly and others added 9 commits May 2, 2017 11:27

Fix crash when no matching results. String title change and added ano…

448b14a

…ther test

Change line break on method

0720f07

Fix missing result and add test to prove it.

ef1d637

Convert outputs to observables

039b883

Made tests better for search adapter, improved projectFactory to have…

27b7bf2

… sensible project names

Fix checkstyle issues

90d7818

Fix test issues

17dc677

refactored test a bit

b8eed64

Improve design of tests

6b13374

luoser approved these changes May 2, 2017

View reviewed changes

Add hashcode, fix checkstyle issues

5c772bf

coryroy merged commit ecb0224 into master May 2, 2017

coryroy deleted the update_search_result_list branch May 2, 2017 22:21

luoser mentioned this pull request May 8, 2017

Search UI pass #99

Merged

1 task


		((KSApplication) view.getContext().getApplicationContext()).component().inject(this);

		scheduler.advanceTimeBy(300, TimeUnit.MILLISECONDS);

		projectList.assertValueCount(2);

Improve Search Result page #85

Improve Search Result page #85

Conversation

coryroy commented Apr 25, 2017

What

coryroy commented Apr 28, 2017

mbrandonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryroy May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luoser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coryroy May 1, 2017 •

edited

Loading