Feature/3750 stats hide show toolbar

mzorz commented 8 years ago

Addresses https://github.com/wordpress-mobile/WordPress-Android/issues/3750

Now the timeframe Spinner is made part of the subtitle toolbar that can be hidden/shown according to scroll down/up using CoordinatorLayout. Also follows the design guidelines as been implemented in the Comments section recently (for reference, check https://github.com/wordpress-mobile/WordPress-Android/pull/3729).

Screenshots

Fully visible :point_down:

screenshot_2016-02-17-18-23-21

Hiding :point_down:

screenshot_2016-02-17-18-23-33

Completely hidden :point_down:

screenshot_2016-02-17-18-39-07

Spinner dropdown open :point_down:

screenshot_2016-02-17-18-23-41

daniloercoli commented 8 years ago

I didn't review the code, I've just taken a look at files changed from mobile. It seems that we need to apply the same fix to stats_activity.xml in sw720dp.

mzorz commented 8 years ago

Thanks for pointing that out @daniloercoli @nbradbury - should be fixed now

nbradbury commented 8 years ago

This looks good to me. @daniloercoli Can you give it a spin and make sure you're comfortable with the changes?

daniloercoli commented 8 years ago

Code looks good with me. Nice work!
I worked on Stats for so long that any change we applied to it feels a little bit off to me. In this particular case I feel weird having 3 toolbars stacked one below the other with almost the "same" background color. Also, with these new changes in, we're not changing the total space available to the "real" Stats content. When you scroll down the timeperiod selector hides, but the main toolbar is still there.

wp-1455786448030

Wonder why we want to apply these changes then, consistency with the rest of the app? If so, what's about the Reader?

cc @mattmiklic

mzorz commented 8 years ago

Code looks good with me. Nice work!

thanks! :)

I worked on Stats for so long that any change we applied to it feels a little bit off to me.

I know that feeling! :smile: Yes totally agreed proposals like this need be discussed more frequently to keep up-to-date :+1:

I think I can answer some of your questions! let me address them here

Wonder why we want to apply these changes then, consistency with the rest of the app?

Great question indeed - short answer is yes; the very same situation regarding status bar + Actionbar + Toolbar we had for Comments. It looked exactly the same as Stats: a Spinner within the action bar.

I can give some highlights of what we found could be improved / what triggered this change:

“materialized” the design
the screen provided no context (title) to the user
the status bar and action bar still are there, as is required for most inner activities (it has a visible back button for navigation, giving the idea of depth too)
the toolbar now only shows when you scroll back up, given it’s likely you might want to change the filter after having scrolled down, then up again.
it’s a common, expected pattern on Android apps for scrollable content these days

For the full discussion, it’s available here: https://github.com/wordpress-mobile/WordPress-Android/pull/3691#issuecomment-179965189 And the solution/implementation explanation is here: https://github.com/wordpress-mobile/WordPress-Android/pull/3729

Also to keep in mind, originally it was also triggered by the need to decouple the functionality from the activity and place it in a fragment as needed for a tablet rewrite (dual pane layout) as per this https://github.com/wordpress-mobile/WordPress-Android/issues/3643 but given that tablet path has been delayed as better careful thought was needed I thought it was OK in this specific Stats case to implement the hiding toolbar right there in the Activity itself to prevent too much change from happening at once (which could lead to potential unstability and make it easier for further discussion with people who participated / owned the feature).

Finally:

If so, what's about the Reader?

Yes! it would make perfect sense to change it on other parts of the app such as reader, for instance too (check @nbradbury’s comment here https://github.com/wordpress-mobile/WordPress-Android/pull/3745#issuecomment-184209927) - something for which we’ve written a component with the purpose of filtering a RecyclerView (called FilteredRecyclerView)

daniloercoli commented 8 years ago

You Win :)

I can give some highlights of what we found could be improved / what triggered this change:

“materialized” the design the screen provided no context (title) to the user the status bar and action bar still are there, as is required for most inner activities (it has a visible back button for navigation, giving the idea of depth too) the toolbar now only shows when you scroll back up, given it’s likely you might want to change the filter after having scrolled down, then up again. it’s a common, expected pattern on Android apps for scrollable content these days

Is it real an expected pattern on Android apps for scrollable content these days? To be clear, I'm not arguing against your implementation, just want to put some more light on the new design. TBH I did't see this patter used in any of my most used apps. Do we have a real example other than "Material" specs? I checked the comments here #3691, and @mattmiklic brought up the Maps app as example, but 2 of theirs toolbars have the same background color, and those seems like one toolbar. Also, it's buried in the Drawer.

mzorz commented 8 years ago

You Win :)

LOL :p

Is it real an expected pattern on Android apps for scrollable content these days? To be clear, I'm not arguing against your implementation, just want to put some more light on the new design. TBH I did't see this patter used in any of my most used apps. Do we have a real example other than "Material" specs?

You're correct in arguing whether it is expected or not, as my expression is stating something on behalf of "all users" and it's not fair to say the least - thanks for pointing that out.

I just looked into my phone, few examples:

Slack app: the toolbar subtitle doesn't hide (as in the Maps app Matt brought as an example) but it's there providing context along with the main action bar.
Google Play app also has a hiding toolbar (it hides the whole action bar in the main screen, but it's the main screen giving content all the space, not an in-depth screen).
Instagram for Android has this pattern in the Activity tab (fourth tab from the left).

We might find more examples to support or to not support this idea but following the core spirit of your question, I think we should be also looking into what kind of users we have and whether or not this is adding value.

Now, turning things around, would you say this is, on the contrary, an unexpected behavior for the user, that takes the user by surprise? I'm giving the material design guidelines some good weight in this case.

mattmiklic commented 8 years ago

There are still a couple of issues with the dropdown button --

The gray arrow in the lower right corner doesn't match the current Material Design guidelines for a dropdown button
The text label of the dropdown button should line up with the title in the top bar.

I mentioned this in #3745:

I missed the question about the arrow icon before the menu was merged in, but I have a question about how it's implemented. The bottom-right-pointing arrow looks standard for the "spinner" control I see in the Android developer documentation. The design actually uses what the Material Design guidelines call a "dropdown button", though. I'd assumed that that downward-pointing arrow included in the Material Design guidelines was a standard system element. I'm having some trouble finding developer documentation on anything called a "dropdown button," though.

mzorz commented 8 years ago

@mattmiklic thanks for jumping in, I'll expand on these two bullets below:

On the arrow icon

The gray arrow in the lower right corner doesn't match the current Material Design guidelines for a dropdown button

@mattmiklic I actually referenced this very aspect in https://github.com/wordpress-mobile/WordPress-Android/pull/3729#issue-132566816 (specifically under the "Design" section), invoking you :) Copying it here:

@mattmiklic Please note the down-pointing arrow is not implemented on purpose - we would need a 9-patch drawable to make it part of the background just like the drawable being used in other Spinners in the app (@drawable/spinner_background_ab_wordpress). Assuming you're the right person to ask for, can you provide these assets, please? If not please advise - I can probably help myself otherwise too. Thanks in advance

I'll happily replace the asset with a new 9patch one when provided.

On the placement of the dropdown listview window

The text label of the dropdown button should line up with the title in the top bar.

Right - the native Spinner component in Android while very handy does not comply 100% with the material design guidelines for dropdown buttons here https://www.google.com/design/spec/components/buttons.html#buttons-dropdown-buttons. Specifically, if we follow the guidelines to the letter, not only the popup window should be aligned with the button, but also when the button is touched and the listview is rendered, the selected item text is suggested to be displayed using the accent color. However, the Spinner control as is doesn't provide this functionality right off the shelf. This can be done of course, but it's just more of a hurdle.

Programmatically, the nearest solution (from a UI perspective) to have both the selected item visible while the available options are displayed as well, is to separate the anchor, as @nbradbury suggested in the third bullet on this comment https://github.com/wordpress-mobile/WordPress-Android/pull/3729#issuecomment-182159546 (and incidentally @maxme has just pointed out how to revert that on another comment here https://github.com/wordpress-mobile/WordPress-Android/pull/3765/files#r53341612).

Taking all of this into consideration, I have a question:

do we want to comply with material design 100% for this or is it OK to iteratively move towards it? it is my assumption that the background dropdown arrow icon shouldn’t be a blocker for this issue, but in the case it is, then the colouring should also be taken into account, right? Maybe we can open a new issue for this?

Looking forward to getting to an agreement on this soon :) - and in the case we do, I guess my next thing would be to implement the use of FilteredRecyclerView in the reader too

Thanks for reading!

PS just an FYI, this points to the wrong issue/pr:

I mentioned this in #3754:

mattmiklic commented 8 years ago

I actually referenced this very aspect in #3729 (comment) (specifically under the "Design" section), invoking you :)

That was what I was replying to in the text I quoted from #3745. :) As I said there, I was under the impression that the arrow graphic would be provided by the system, since it is a native material design icon. I can export a set of PNGs for you, though.

do we want to comply with material design 100% for this or is it OK to iteratively move towards it?

Our goal is to embrace material design as fully as possible; we should aim for this dropdown button to match the material design spec, but it's ok if it has to happen iteratively, as long as progress continues to be made on that point.

it is my assumption that the background dropdown arrow icon shouldn’t be a blocker for this issue, but in the case it is, then the colouring should also be taken into account, right?

I don't quite understand what you mean; I'm assuming I'll give you a white icon to match the design, but please clarify if I'm misunderstanding.

mattmiklic commented 8 years ago

Here is the arrow asset you'll need for the dropdown button: arrow.zip

mzorz commented 8 years ago

That was what I was replying to in the text I quoted from #3745. :) As I said there, I was under the impression that the arrow graphic would be provided by the system, since it is a native material design icon. I can export a set of PNGs for you, though.

LOL - and I also already replied to that there too:

https://github.com/wordpress-mobile/WordPress-Android/pull/3745#issuecomment-183962770 (copying here)

Regarding the icon for drop downs, it’s ok to just leave the one being used on spinners anywhere else. What I was saying in my comment in the other issue is that, in case we really want to use the pointing-down-triangle icon, I’d need it to be resolved with 9-patch to use it in the same manner that the current Spinner is implemented. I can replace the asset if provided.

Re: this: thanks for the file, I'll prepare the 9 patch myself.

it is my assumption that the background dropdown arrow icon shouldn’t be a blocker for this issue, but in the case it is, then the colouring should also be taken into account, right?

I don't quite understand what you mean; I'm assuming I'll give you a white icon to match the design, but please clarify if I'm misunderstanding.

Sure - implementing material design here means we need to add the proper icon but also the proper background coloring, selected states, ripple effect, etc. and all this needs proper definition.

I would suggest we can open a new issue to discuss material design application on this specific case, sounds good?

I'll prepare the 9 patch sometime today or tomorrow and push it back.

Thanks!

mattmiklic commented 8 years ago

implementing material design here means we need to add the proper icon but also the proper background coloring, selected states, ripple effect, etc. and all this needs proper definition.

Thanks for clarifying -- I think it would be fine to improve this later in another issue, getting the correct arrow icon in is I think the most important part since the standard spinner's corner arrow is so distinctive.

mzorz commented 8 years ago

:+1:

mzorz commented 8 years ago

Changing the arrow icon turned out to be even easier than I thought - I didn't need use a 9patch, as I set the background for the Spinner to be transparent and then added a drawableRight to the filter_spinner_item layout :smile:

I noticed the Spinner width changes as you pick different options, but I think this is normal behavior (either we wrap_content as it is right now, or we set it to full width occupying the whole layout width, which is not intended). If we want to give it a fixed width in dp so no matter which option is selected ithe Spinner remains the same width, please suggest what the value for that should be @mattmiklic .

Posting a couple screenshots here (the same is applied in the Comments section)

screenshot_2016-02-19-13-31-11 screenshot_2016-02-19-13-31-15

mattmiklic commented 8 years ago

If we want to give it a fixed width in dp so no matter which option is selected ithe Spinner remains the same width, please suggest what the value for that should be @mattmiklic .

I think the variable width is preferable in this context -- from looking at the specs I think a dropdown button should be limited to the width of its text label when used in a toolbar. So no change needed I think, looks good!

It does look like it could benefit from moving to the left about 4dp, to better line up the dropdown button text with the title above it.

mzorz commented 8 years ago

It does look like it could benefit from moving to the left about 4dp, to better line up the dropdown button text with the title above it.

Oh that's an inner offset the Spinner has - take a look at the fully displayed dropdown list (https://cloud.githubusercontent.com/assets/6597771/13182226/7e530c04-d70f-11e5-8289-8fcbda055468.png), it's aligned to the left with the title above it. Might give it a try though, this is just the default.

mattmiklic commented 8 years ago

That makes sense -- it would be really nice if we could adjust it so that it optically lines up, though it's not a huge deal.

mzorz commented 8 years ago

:+1:

mzorz commented 8 years ago

Here it is! 4dp less in the margin so it optically lines up

screenshot_2016-02-19-14-34-35 screenshot_2016-02-19-14-34-41

mattmiklic commented 8 years ago

Here it is! 4dp less in the margin so it optically lines up

Perfect! :+1:

mzorz commented 8 years ago

@daniloercoli @nbradbury should we agree on merging this one?

nbradbury commented 8 years ago

I'm good to merge. @daniloercoli OK with you?

daniloercoli commented 8 years ago

Good to merge!

nbradbury commented 8 years ago

:shipit:

wordpress-mobile / WordPress-Android