Feature/reader full post refresh related posts by nbradbury · Pull Request #4626 · wordpress-mobile/WordPress-Android

nbradbury · 2016-10-09T00:35:41Z

This is the third in a slew of PRs which tackles the full post refresh (#4581). This PR deals with upgrading the related posts shown below every wp.com post in the reader post detail.

Previously we showed a single "related reading" section, now we show two related posts from the same site as the current post ("local" related posts), followed by two related posts from across wp.com ("global" related posts).

The before & after pics below illustrate the changes.

…ss-Android into feature/reader-full-post-refresh-related-posts

…hub.com/wordpress-mobile/WordPress-Android into feature/reader-full-post-refresh-related-posts

hypest

Noticed that the images on the related posts are vertically stressed. I could attach a screenshot if you want but, the issue is visible on the shot on the PR description as well.

hypest

On my network connection, Related posts take a few seconds to load and there is nothing to indicate that there's a background attempt in loading them.

May I suggest making the headers ("MORE IN ..." and "MORE ON WORDPRESS.COM" always visible and add some form of indicator that filling in those sections is still undergoing (e.g. a progress indicator)? Thanks!

nbradbury · 2016-10-12T11:12:21Z

Noticed that the images on the related posts are vertically stressed. I could attach a screenshot if you want but, the issue is visible on the shot on the PR description as well.

Bah, you're right and I meant to mention that. If it's okay with you, I'd like to open a separate issue for this. The fix is likely a bit convoluted and this PR is already fairly large.

nbradbury · 2016-10-12T11:15:31Z

May I suggest making the headers ("MORE IN ..." and "MORE ON WORDPRESS.COM" always visible and add some form of indicator that filling in those sections is still undergoing (e.g. a progress indicator)?

I don't think we want the headers visible since there are cases when one (or, rarely, both) won't have related posts. So we only want to show them when we know they have related posts, which doesn't happen until after the call completes.

I'm ambivalent about the progress indicator. I don't think it's really needed but it's simple enough to add. Just let me know if you feel that would necessary.

hypest · 2016-10-12T11:15:33Z

If it's okay with you, I'd like to open a separate issue for this. The fix is likely a bit convoluted and this PR is already fairly large.

@nbradbury sure, WFM!

hypest · 2016-10-12T11:24:56Z

I don't think we want the headers visible since there are cases when one (or, rarely, both) won't have related posts. So we only want to show them when we know they have related posts, which doesn't happen until after the call completes.

I'm ambivalent about the progress indicator. I don't think it's really needed but it's simple enough to add. Just let me know if you feel that would necessary.

I see. I can see that needing a bit of effort that could be delivered with a subsequent PR so, I'd say let's just make this a ticket instead. Besides, the same happens with the "Liked by" section (invisible until response arrives) so, perhaps we can include that section as well in the same ticket.

hypest

Hey @nbradbury , I've left some more comments, most of which are non-blocking. I'd like to hear your opinion on them though before moving on. Thanks!

hypest · 2016-10-12T12:16:36Z

WordPress/src/main/java/org/wordpress/android/ui/reader/models/ReaderRelatedPost.java

+        ReaderRelatedPost post = new ReaderRelatedPost();
+
+        post.mPostId = json.optLong("ID");
+        post.mSiteId = json.optLong("site_ID");


Hmm, are ID and site_ID really optional on the server side?

If not, I would prefer us to use getLong and handle the errors instead.

I've favored using the various optXxx methods throughout the reader, but I'm certainly open to alternatives. How would you suggest I handle the errors?

In this case, I'd say to drop the specific related post json and return null. Caller will need to check for the null accordingly. WDYT?

hypest · 2016-10-12T12:29:19Z

WordPress/src/main/java/org/wordpress/android/ui/reader/models/ReaderRelatedPostList.java

+        }
+
+        ReaderRelatedPostList posts = new ReaderRelatedPostList();
+        JSONArray jsonPosts = json.optJSONArray("posts");


This made me pause a bit, wondering why would we need to dive deeper into the json object to get the data we need for this function.

May I suggest removing the need for this knowledge (of where the list is contained into the json structure) and pass the JSONArray directly as the function parameter? The caller (handleRelatedPostsResponse, which is a network response handler in this case) can validate the json and offer the list directly. Let me know what you think!

Agreed. Changed in e57687f.

hypest · 2016-10-12T12:53:14Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderRelatedPostsView.java

+
+    private void initView(Context context) {
+        inflate(context, R.layout.reader_related_posts_view, this);
+        mFeaturedImageWidth = DisplayUtils.dpToPx(getContext(), getResources().getDimensionPixelSize(R.dimen.reader_related_post_image_width));


Can we reconcile the 2 Contexts used in these lines? I mean, there is one passed as a parameter and one got via getContext() so, can we instead use only one or the other? Thanks!

Oops! Good catch - fixed in 8521709.

hypest · 2016-10-12T13:05:12Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderRelatedPostsView.java

+        mClickListener = listener;
+    }
+
+    public void showRelatedPosts(ReaderRelatedPostList posts, String siteName, boolean isGlobal) {


Function seems to expect posts being not null so, we might as well denote it with an annotation like in other places in the PR, what do you think?

Added in eb101af.

hypest · 2016-10-12T13:12:09Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderRelatedPostsView.java

+
+    public void showRelatedPosts(ReaderRelatedPostList posts, String siteName, boolean isGlobal) {
+        mRelatedPostList = posts;
+        if (mRelatedPostList.size() == 0) return;


Interesting exit from the function.

I'm wondering, for completeness sake's would it be better to clean up any elements that might already be in the View's layout? Like, if showRelatedPosts() is called again basically but with an empty list. What do you think?

Makes total sense - added in 185662b.

hypest · 2016-10-12T13:30:20Z

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


-    <LinearLayout
+    <RelativeLayout
+        android:id="@+id/layout_related_post_site_header"


This is more like a comment just to put it out there: It seems that the header click acts as a click on the related post itself and opens the post. It felt counter-intuitive the first couple of times I tried it. I was expecting it to go to the site (in Reader) instead. Apart from the smaller size overall, the header looks like the relevant headers elsewhere, and there the click opens the site.

This is not a blocking comment though.

I wrestled with this one. I agree it would be more consistent to show the site when the header is clicked, but at the same time it's very easy to tap the header when you intended to tap the post (something that actually happens to me already in the list view, but that's another story!). So in the end I decided not to make the header tappable.

I see. I think this is a good comment (augmented by your hands-on feedback as well) that we prolly should raise it at the design review phase as well. This is, again, a case where the design for the web (where a mouse will be used to accurately click on the various sections) is not suitable for the mobile where (big) fingers demand ample spacing.

hypest · 2016-10-12T13:51:30Z

Another small note, not a blocking one as well: the Follow button doesn't provide any hints of its function. It is familiar from the rest of the app but still. On Calypso web, the button has a tooltip appearing on mouse-over which of course is not directly translatable to the mobile.

… empty

nbradbury · 2016-10-12T15:06:05Z

Another small note, not a blocking one as well: the Follow button doesn't provide any hints of its function. It is familiar from the rest of the app but still. On Calypso web, the button has a tooltip appearing on mouse-over which of course is not directly translatable to the mobile.

Yes, that bothered me as well. I originally had the follow/following text on that button, but again it complicated the tap target. It was easy to tap the follow text when you meant to tap the post, especially on smaller screens, so I dropped the follow text (which matches the mobile web design, too).

I think the purpose of that button is obvious enough, though, since as you mention it's shown in so many other places.

nbradbury · 2016-10-12T15:08:08Z

Thanks for reviewing this one @hypest - ready for round two!

…hub.com/wordpress-mobile/WordPress-Android into feature/reader-full-post-refresh-related-posts

hypest

Hey @nbradbury , thanks for the extra commits! I've left some new comments that probably require changes so, have a look. Thanks!

hypest · 2016-10-13T08:44:22Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderRelatedPostsView.java

+
+    private void initView(Context context) {
+        inflate(context, R.layout.reader_related_posts_view, this);
+        mFeaturedImageWidth = DisplayUtils.dpToPx(context, getResources().getDimensionPixelSize(R.dimen.reader_related_post_image_width));


I the same vein as the previous comment in this function, perhaps we better use the provided context to retrieve the Resources. So, something like context.getResources(). WDYT?

Addressed in 10be46e

hypest · 2016-10-13T08:54:18Z

WordPress/src/main/java/org/wordpress/android/ui/reader/models/ReaderRelatedPost.java

+        ReaderRelatedPost post = new ReaderRelatedPost();
+
+        post.mPostId = json.optLong("ID");
+        post.mSiteId = json.optLong("site_ID");


In this case, I'd say to drop the specific related post json and return null. Caller will need to check for the null accordingly. WDYT?

hypest · 2016-10-13T08:59:18Z

WordPress/src/main/java/org/wordpress/android/ui/reader/models/ReaderRelatedPostList.java

+    public static ReaderRelatedPostList fromJsonPosts(@NonNull JSONArray jsonPosts) {
+        ReaderRelatedPostList posts = new ReaderRelatedPostList();
+        for (int i = 0; i < jsonPosts.length(); i++) {
+            posts.add(ReaderRelatedPost.fromJson(jsonPosts.optJSONObject(i)));


Didn't notice that on my first pass but, it seems ReaderRelatedPost.fromJson() is not expecting null as a parameter but jsonPosts.optJSONObject can return one. We prolly need to null check the parameter first, or change the function to accept null, right?

I don't think it's possible for optJSONObject() to return null here, since we're looping through the array of objects. But for future use null-checking makes sense - added in 69c0909.

AFAICT, optJSONObject() can return null if the array item is not a JSONObject and JSONArray can indeed hold a mix of any kind of json types. The null check you added will do the job so, 👍

hypest · 2016-10-13T09:06:36Z

Yes, that bothered me as well. I originally had the follow/following text on that button, but again it complicated the tap target. It was easy to tap the follow text when you meant to tap the post, especially on smaller screens, so I dropped the follow text (which matches the mobile web design, too).

I think the purpose of that button is obvious enough, though, since as you mention it's shown in so many other places.

Hopefully we can bring this up at the design review phase as well. Thanks!

nbradbury · 2016-10-13T12:11:56Z

In this case, I'd say to drop the specific related post json and return null. Caller will need to check for the null accordingly.

Addressed in 419aa41. Ready for round 3!

hypest · 2016-10-13T12:15:33Z

LGTM @nbradbury , thanks for the changes! I'll merge this as soon as Travis has its pass. Thanks!

nbradbury added 30 commits October 7, 2016 11:57

Added separate routines for requesting local/global related posts

bdaf85a

Separate container views for local/global

c29a20a

Separate label views for local/global

c23f357

Reduce font size

3dcc500

Show site name in local related posts

bd55106

No divider between related posts

6b0fa7b

Show excerpt rather than byline

7f593df

Show byline only for global

3b882be

Simplify related posts by only requesting & parsing necessary fields

cc835d1

Flattened heirarchy or related post view

d65c124

First pass at site header for global related posts

9ceb179

Removed featured image

acd35fd

Moved related posts to a separate view object

515b817

Re-implemented click listener

de88c29

Removed background & padding, added dividers

40943c8

Margin cleanup, fixed gravatar bug

20b661c

Added back featured image

a7770d8

Reversed author name & site name in the header

0fefbfd

Don't show featured image unless there's an excerpt

0ca61a5

Added RELATED_POST_FIELDS constant

6b84144

Moved featured image to the left

d72b4cf

Removed unused background drawable

b8e579c

Make featured image fit the height of the related post view

cab591f

First pass at follow button

7d5e08f

2nd pass at follow button

1f7b6e9

Modify follow button to have no caption

fabbe9d

Correct bug that caused site header on local related posts

e018e61

Increased featured image width

68454e7

Request local related posts first

ed23a6b

Use single call to get all related posts at once

c6c065f

nbradbury added 3 commits October 11, 2016 20:34

Merge branch 'develop' of https://github.com/wordpress-mobile/WordPre…

c984492

…ss-Android into feature/reader-full-post-refresh-related-posts

Merge branch 'feature/reader-full-post-refresh-master' of https://git…

6ec376b

…hub.com/wordpress-mobile/WordPress-Android into feature/reader-full-post-refresh-related-posts

Merge branch 'feature/reader-full-post-refresh-master' of https://git…

db19a10

…hub.com/wordpress-mobile/WordPress-Android into feature/reader-full-post-refresh-related-posts

hypest suggested changes Oct 12, 2016

View reviewed changes

nbradbury mentioned this pull request Oct 12, 2016

Reader: stretched images in full post refresh #4640

Closed

hypest suggested changes Oct 12, 2016

View reviewed changes

nbradbury added 4 commits October 12, 2016 10:29

Pass JSON post array when creating ReaderRelatedPostList

e57687f

Always use passed context in initView

8521709

Added @NonNull annotation to showRelatedPosts

eb101af

Remove all views from the container before checking if passed list is…

185662b

… empty

Merge branch 'feature/reader-full-post-refresh-master' of https://git…

7a5a202

…hub.com/wordpress-mobile/WordPress-Android into feature/reader-full-post-refresh-related-posts

hypest suggested changes Oct 13, 2016

View reviewed changes

nbradbury added 3 commits October 13, 2016 07:57

Use context.getResources()

10be46e

Null check ReaderRelatedPost.fromJSON

69c0909

Use getLong

419aa41

hypest approved these changes Oct 13, 2016

View reviewed changes

hypest merged commit 01b30aa into feature/reader-full-post-refresh-master Oct 13, 2016

hypest deleted the feature/reader-full-post-refresh-related-posts branch October 13, 2016 12:25

nbradbury mentioned this pull request Oct 13, 2016

Feature/reader full post refresh featured image #4652

Merged

Conversation

nbradbury commented Oct 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

nbradbury commented Oct 12, 2016

Uh oh!

nbradbury commented Oct 12, 2016

Uh oh!

hypest commented Oct 12, 2016

Uh oh!

hypest commented Oct 12, 2016

Uh oh!

hypest left a comment

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!

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!

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!

hypest commented Oct 12, 2016

Uh oh!

nbradbury commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbradbury commented Oct 12, 2016

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

hypest Oct 13, 2016 • 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!

hypest commented Oct 13, 2016

Uh oh!

nbradbury commented Oct 13, 2016

Uh oh!

hypest commented Oct 13, 2016

Uh oh!

Reviewers

nbradbury commented Oct 9, 2016 •

edited

Loading

nbradbury commented Oct 12, 2016 •

edited

Loading

hypest Oct 13, 2016 •

edited

Loading