Async media: Scroll to failed post from error notifications #6424

aforcier · 2017-07-28T16:28:29Z

Addresses a suggestion from @iamthomasbishop (marked in the to-do list here).

#6420 should be merged before merging this PR.

When tapping a notification for a failed post upload, the post list is opened. Instead of showing a dialog, the list will now scroll down to the post that failed, and a dialog is no longer shown.

@iamthomasbishop it looks to me like it would be better to scroll a bit more, so that the post appears at the top of the list (rather than scroll just enough to reveal it at the bottom as in the demo above). I can change that but waiting for any more feedback on that you might have before diving in.

cc @mzorz

iamthomasbishop · 2017-07-28T17:41:35Z

it looks to me like it would be better to scroll a bit more, so that the post appears at the top of the list

Agreed. I'm starting to think we should also add an outline of the posts w/ their associated alert color (error = red, alert = yellow, etc), but let's obviously hold on that idea.

aforcier · 2017-07-28T18:05:37Z

@iamthomasbishop updated to display the post is at the top of the list instead:

mzorz · 2017-07-28T20:11:10Z

one thing that I'm not sure about here is that when you tap the notification and you're shown the list of posts, with the error one being the first one shown, it looks like that is actually the absolute first in the list. We do have the subtle hint that it is not because the scrollbar appears and disappears on the right, but still wondering if we could maybe not make it the top most item but center it in the screen, or maybe just show a little offset so the item above it shows a bit, to hint the user there's more stuff up there? Just thinking aloud here, what do you think @iamthomasbishop ?

iamthomasbishop · 2017-07-28T20:35:29Z

@mzorz I very much agree. If we could have the post before it hang 10-20dp off the top there, that'd be helpful.

Another option - start at the top of the list when the page loads, and smooth scroll quickly to that post.

mzorz · 2017-07-28T21:06:12Z

What if we leave it as is, but make an animation on the alpha highlighting the item to bring the user's attention?

iamthomasbishop · 2017-07-28T21:57:34Z

@mzorz I'm okay with that!

…r-scroll-to-post

aforcier · 2017-07-31T11:45:46Z

@iamthomasbishop @mzorz I did two options, the smooth scroll (can be made slower than this), and the alpha highlight. The alpha one is maybe going to be a bit more error prone as it had to be done in a somewhat hacky way.

Smooth scroll:

Alpha highlight/blink:

folletto · 2017-07-31T12:00:39Z

The alpha one feels almost like a glitch to me, not a highlight.

The scrolling one seems solid.

mzorz · 2017-07-31T13:24:01Z

LGTM @iamthomasbishop if you agree I'll merge 🙇

iamthomasbishop · 2017-07-31T14:31:15Z

@aforcier @mzorz That's much better! Very small request - Can we make the post before the error'd one hang off the top of the screen 10-20dp? Very rough sketch for reference: https://cloudup.com/c3hjt8YwZhD

aforcier · 2017-07-31T15:10:22Z

@iamthomasbishop sure! Here are 10dp, 20dp, and 40dp (that's including the margin between the posts, so it's not exact):

mzorz · 2017-07-31T15:11:22Z

I like 20dp the best

aforcier · 2017-07-31T15:43:13Z

@iamthomasbishop asked to bump the offset a bit to 23dp. Pushed the change for that in 711f21e. Here's what it looks like:

Design review is done @mzorz, ready for a new code pass 🎉

mzorz · 2017-07-31T17:46:11Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.java

            PostsListFragment oldFragment = mPostList;
-            mPostList = PostsListFragment.newInstance(mSite, mIsPage);
+            mPostList = PostsListFragment.newInstance(mSite, mIsPage, targetPost);


PostsListFragment.newInstance() only uses targetPost to obtain it's id and set a parcelable argument. Later on that id will be properly used to retrieve the actual PostModel from the db when it is actually needed.
Also, the only reason why PostStore is imported and injected here is to be able to get the PostModel.

Why don't we just remove the use of mPostStore in PostsListActivity altogether, and pass the ID here as well?

This would also be in line with the decision taken on trying to avoid this kind of PostModel passing around because of potential OOM problems, like in this PR here #5856

In this case we're passing the PostModel directly to a method, it's not being parceled, so #5856 shouldn't apply.

I decided to use the model for two reasons:

We won't create a new instance of the PostsListFragment if the post has since been deleted (since it would have a non-0 id, but the PostModel obtained from the store would be null)

This avoids PostsListFragment.newInstance() taking an int id argument, which is error-prone and the main case for using models over ids

In this case we're passing the PostModel directly to a method, it's not being parceled, so #5856 shouldn't apply.

Forgive me, I wasn't clear: I was trying to acknowledge how the same treatment to this case is given within the fragment by using the ID instead of the PostModel. I should have said something like "it's great to see we're using this in the same way here".

Having discussed these 2 points in Slack, and acknowledging point 1 is better kept as is as per the advantage of keeping fragments, and leaving point 2 for a longer discussion elsewhere, here's one minor thing users could benefit from:

If the user taps on an error notification for which the referred PostModel doesn’t exist anymore (i.e. has been deleted) we should do something else then, maybe show a Toast or something, indicating the Post wasn’t found.

To test:

start draft

make it fail

delete draft

tap on notification

Added toast in fece2bf.

mzorz · 2017-07-31T19:13:04Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.java

@@ -21,13 +21,14 @@

 public class PostsListActivity extends AppCompatActivity {
    public static final String EXTRA_VIEW_PAGES = "viewPages";
-    public static final String EXTRA_ERROR_MSG = "errorMessage";
+    public static final String EXTRA_TARGET_POST_ID = "targetPostId";


Could we rename this to EXTRA_TARGET_POST_LOCAL_ID - I'd be happier :)

🤦‍♂️ yes, good catch

Done in 3e0b6f6.

aforcier · 2017-07-31T19:21:35Z

Ready for another pass I believe @mzorz.

mzorz · 2017-07-31T19:24:19Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java

+                final int position = mPostsListAdapter.getPositionForPost(mTargetPost);
+                if (position > -1) {
+                    RecyclerView.SmoothScroller smoothScroller = new LinearSmoothScroller(getActivity()) {
+                        private static final int SCROLL_OFFSET_DP = 23;


Can we add a comment here explaining why we are moving this offset? maybe something like // we're showing just a bit of the item above to hint the user they can navigate up (feel free to re-write to something better worded than this)

mzorz · 2017-07-31T19:31:47Z

There's one thing I found: if you rotate the device the List is always scrolled to the top. So, say the problematic post is somewhere in the middle, it fails, you tap on the error Notification and are correctly scrolled to it, but then rotate the screen, the list loses its position and the first item is shown.

To test:

choose from an old draft in your list (something that is below or in the middle of your list)
modify it and make it fail sync
see the notification appear
scroll the item away of your post list
tap on the notification
observe you're taken to the correct item
now rotate the screen
the first item is shown

mzorz · 2017-07-31T20:40:33Z

I confirmed this is happening in develop, so opening a new issue here #6437

mzorz · 2017-07-31T20:40:44Z

LGTM

aforcier added 3 commits July 28, 2017 11:34

Pass the post when opening post list from error notification

e70fc2a

Scroll list to failed post when opening error notification

3bf6a80

Remove post list error dialog (triggered from error notifications)

d108595

aforcier added the Post List label Jul 28, 2017

iamthomasbishop mentioned this pull request Jul 28, 2017

Async media: Design and copy updates #6420

Merged

5 tasks

Scroll target post to the top of the list

56d8d25

Merge branch 'issue/6386-async-design-tweaks' into feature/async-erro…

13de017

…r-scroll-to-post

aforcier added the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Jul 31, 2017

Smooth scroll to failed post

08c6954

aforcier mentioned this pull request Jul 31, 2017

Async ('Upload and Publish') MVP Design and Copy Review #6386

Closed

aforcier changed the base branch from issue/6386-async-design-tweaks to feature/async-media July 31, 2017 13:30

aforcier mentioned this pull request Jul 31, 2017

Async Design Master Issue #6130

Closed

12 tasks

Offset scroll-to-post to show part of above post

711f21e

aforcier assigned mzorz Jul 31, 2017

aforcier removed the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Jul 31, 2017

mzorz reviewed Jul 31, 2017

View reviewed changes

aforcier added 2 commits July 31, 2017 15:19

Rename target post id constant

3e0b6f6

Show toast when tapping error notification for unknown post

fece2bf

mzorz reviewed Jul 31, 2017

View reviewed changes

mzorz merged commit b0ddcf3 into feature/async-media Jul 31, 2017

aforcier deleted the feature/async-error-scroll-to-post branch August 1, 2017 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async media: Scroll to failed post from error notifications #6424

Async media: Scroll to failed post from error notifications #6424

aforcier commented Jul 28, 2017 •

edited

Loading

iamthomasbishop commented Jul 28, 2017

aforcier commented Jul 28, 2017

mzorz commented Jul 28, 2017

iamthomasbishop commented Jul 28, 2017

mzorz commented Jul 28, 2017

iamthomasbishop commented Jul 28, 2017

aforcier commented Jul 31, 2017

folletto commented Jul 31, 2017

mzorz commented Jul 31, 2017

iamthomasbishop commented Jul 31, 2017

aforcier commented Jul 31, 2017

mzorz commented Jul 31, 2017

aforcier commented Jul 31, 2017

mzorz Jul 31, 2017

aforcier Jul 31, 2017

mzorz Jul 31, 2017

mzorz Jul 31, 2017

aforcier Jul 31, 2017

mzorz Jul 31, 2017

aforcier Jul 31, 2017

aforcier Jul 31, 2017

aforcier commented Jul 31, 2017

mzorz Jul 31, 2017

mzorz commented Jul 31, 2017

mzorz commented Jul 31, 2017

mzorz commented Jul 31, 2017

Async media: Scroll to failed post from error notifications #6424

Async media: Scroll to failed post from error notifications #6424

Conversation

aforcier commented Jul 28, 2017 • edited Loading

iamthomasbishop commented Jul 28, 2017

aforcier commented Jul 28, 2017

mzorz commented Jul 28, 2017

iamthomasbishop commented Jul 28, 2017

mzorz commented Jul 28, 2017

iamthomasbishop commented Jul 28, 2017

aforcier commented Jul 31, 2017

Smooth scroll:

Alpha highlight/blink:

folletto commented Jul 31, 2017

mzorz commented Jul 31, 2017

iamthomasbishop commented Jul 31, 2017

aforcier commented Jul 31, 2017

mzorz commented Jul 31, 2017

aforcier commented Jul 31, 2017

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

aforcier commented Jul 31, 2017

Choose a reason for hiding this comment

mzorz commented Jul 31, 2017

mzorz commented Jul 31, 2017

mzorz commented Jul 31, 2017

aforcier commented Jul 28, 2017 •

edited

Loading