wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

Login Rework: prologue screen visual touches #6196

Closed hypest closed 6 years ago

hypest commented 6 years ago

Finishes up the visual and textual touches for the login prologue screen.

device

To test:

folletto commented 6 years ago

I think we can proceed with this button style and review later as necessary. 👍

mattmiklic commented 6 years ago

Hi @hypest, this is looking good so far. I have a couple of notes and questions for you based on reviewing the screenshot you provided. If you could make a build available for me to install, I'd love to do that and take a closer look as well.

folletto commented 6 years ago

To confirm from the design discussion (p77Llu-5W9-p2), we proceed with the pure Material buttons styled with our colors.

From @mattmiklic's comment there:

Standard material buttons, with blue_medium as the color for the primary buttons; and either grey_light or white as the background for standard buttons.

hypest commented 6 years ago

Hey @mattmiklic !

If you could make a build available for me to install, I'd love to do that and take a closer look as well

I'll make one and circulate it, cool. Unfortunately, my attempt to setup BuddyBuild for automatic APK creation has failed so far.

Have you implemented the alternating background colors on these screens? As you can see in the mockups, the Reader and Notifications screens should use a blue_medium background with white text

Yeap, that's already implemented.

To do this, in the mockups I used blue_dark, with an opacity of 10%, for the status bar

Awesome, will use that, thanks! EDIT: It seems the status bar can be made translucent but the color is actually picked up by the system. Here's a shot:

status_bar_translucent

Let's adjust the animations so that they each only play once, instead of twice

On it, cool!

folletto commented 6 years ago

I see the updated screenshot... I was trying to get a resource on what were the parameters, and I found: https://developer.android.com/training/material/theme.html#StatusBar

It's not super clear from there what are the limits... it seems that it's either inheriting the color in some way, or fully transparent. I would try to avoid to write custom code here just to get a specific color, so maybe we can pick one the is the closest one to the result we want?

hypest commented 6 years ago

Hey @folletto , the problem is with the alternating background color and the fact that when sliding, both "screens" are on screen. At that phase, the status bar will only have one or the other, making the "illusion" non working.

I have tried setting the colors specified in https://developer.android.com/training/material/theme.html#StatusBar but the status bar doesn't seem to permit transparency (or, the current version of the Support Library has an issue). What finally seemed to work OK-ish is using the windowTranslucentStatus=true theme setting, and that's how I produced the screenshot above. All in all, we either need to drop the alternating color of the prologue screens or we keep the solution in the screenshot. WDYT?

mattmiklic commented 6 years ago

Thanks for the test build, I just had a chance to take a look. The alternating background colors are looking good! And I think the status bar is OK as-is.

In the text, there are a few places where a dash is used; we're currently using a hyphen character, like this: - . We should change these to an em dash: — . This is unicode character \u2014.

I'm comparing these buttons to the ones in the Material Design spec, and they seem small -- 32dp height with ~12sp text. The spec says that raised buttons are 36dp height with 14sp text. Oddly, I tried comparing them to some system apps, and found that buttons in Google's own apps aren't consistent with the material design spec, either. If this can't be changed, we'll go with what we've got, but I wanted to try to make sure we're adhering to the recommended sizes for touch targets.

Here's a comparison of a button in Maps, vs one from Google's Material Design assets for Sketch, vs a screenshot of the current build.

screen shot 2017-07-06 at 4 08 22 pm

Last, I like that each animation is only playing once before we advance to the next screen -- but I think we need to delay the advancing between screens, since there's not enough time to read them currently. Could we have each screen play the animation once, and then advance to the next screen after 10 seconds have passed? Similarly, sometimes if I manually swipe between screens, they'll switch immediately when the animation stops, even if I'm in the process of swiping between them. If this gets too tricky, we could remove the auto-advancing, and just let users swipe between them manually.

theck13 commented 6 years ago

I was able to color the status bar with blue_dark and 10% alpha by replacing <item name="android:windowTranslucentStatus">true</item> with <item name="android:statusBarColor">#19005082</item> in styles.xml and adding getWindow().getDecorView().setSystemUiVisibility(View.SYSTEM_UI_FLAG_LAYOUT_STABLE | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN); to LoginActivity.onCreate(). See the screenshot below for illustration.

status

mattmiklic commented 6 years ago

I was able to color the status bar with blue_dark and 10% alpha

Heck yeah! That looks great @theck13.

hypest commented 6 years ago

@theck13 , very interesting solution that one with the SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN and co! Unfortunately, it makes the activity full screen and more hacks are needed for the other screens. One of the nastier ones has to do with the fact that the layout doesn't resize when the virtual keyboard opens up. So, I'd say let's leave the windowTranslucentStatus=true solution in for now and we might revisit, cool? /cc @mattmiklic

hypest commented 6 years ago

We should change these to an em dash: —

Done with 8e4c68f.

I'm comparing these buttons to the ones in the Material Design spec, and they seem small -- 32dp height with ~12sp text. The spec says that raised buttons are 36dp height with 14sp text.

Checked the implementation and the Library does use 14sp for the button text size and we're not customise it. I also created a new project, targeting API 25 only to use the proper Material Library and the test result is the same. That is, our button (size, text size) matches the API25-only app. Let's leave it as is for this pass and we can revisit and customise later, wdyt @mattmiklic ?

I like that each animation is only playing once before we advance to the next screen -- but I think we need to delay the advancing between screens, since there's not enough time to read them currently. Could we have each screen play the animation once, and then advance to the next screen after 10 seconds have passed?

Yeah, that was the primary reason I left 2 or 3 loops there, to help give time to read the long-ish promo texts. I can try adding a manual delay at the end as you suggest to see if it's too much a trouble or not. Will let you know.

they'll switch immediately when the animation stops, even if I'm in the process of swiping between them

I see, yeah. I will try to guard for that case.

mattmiklic commented 6 years ago

Let's leave it as is for this pass and we can revisit and customise later, wdyt @mattmiklic ?

Sounds fine -- if they're matching what you get right out of the box, it sounds like the inconsistency is between the Material Design spec and the Android libraries, not with us, so we can probably leave these as-is.

hypest commented 6 years ago

As discussed in chat, I've implemented (0c1c0ac) a slightly different but simpler approach for the auto-advancing: the promo screens auto-advance without a delay until the user starts swiping. At that point and on, auto-advancing stops.

hypest commented 6 years ago

I think I've addressed all feedback up to this point so, ready for another pass @mattmiklic and @theck13 , thanks!

hypest commented 6 years ago

Hmm, it seems that more hacks are needed for the other screens is still true for the current solution for the Status Bar translucency.

hypest commented 6 years ago

I've reinstated the older, static Status Bar color solution since with the current implementation of the login screens, it's too messy to have the prologue screen have a translucent Status Bar while the rest of the screens don't.

mattmiklic commented 6 years ago

It still feels like the screens are auto-advancing a little bit too soon to be able to read the screens, and the transition animation also feels too fast. In the interest of simplifying this, why don't we remove the auto-advancing, and just let users swipe between them at their own pace. The auto-advancing is nice but it brings up additional issues to resolve, and I feel like we've spent a lot of time implementing this screen already.

We should make sure each animation only plays once, each time the user swipes to the screen it's on, rather than having them repeat infinitely.

For the status bar, let's use black as its background.

mattmiklic commented 6 years ago

For the status bar, let's use black as its background.

Unless it has to be the same as the login screens, in which case let's just leave it as-is.

hypest commented 6 years ago

In the interest of simplifying this, why don't we remove the auto-advancing, and just let users swipe between them at their own pace. The auto-advancing is nice but it brings up additional issues to resolve, and I feel like we've spent a lot of time implementing this screen already.

Agreed 100% @mattmiklic , removed the auto-advancing with aa79b2c.

We should make sure each animation only plays once, each time the user swipes to the screen it's on, rather than having them repeat infinitely.

Done with 5c99716. Each anim is played only once, everytime each page is shown.

For the status bar, let's use black as its background. Unless it has to be the same as the login screens, in which case let's just leave it as-is.

Yeah, it's actually the same on all of Login screens (except for the epilogue which is a different Android Activity) so, we will need to keep it as is for now. Thanks!

Ready for another pass!

mattmiklic commented 6 years ago

@hypest thanks for making those changes; it's all looking good now. I noticed that the Stats and Notifications screens look a little bare when the animation finishes; I tweaked them so that the illustration doesn't animate back offscreen. This looks better since it's no longer repeating. I've tested these in Lottie Preview so hopefully they'll work on the first try.

Stats: https://cldup.com/PltMKRa9hz.json Notifications: https://cldup.com/OE-Exjq1Gm.json

hypest commented 6 years ago

Updated to the new anims @mattmiklic , they seem to work OK 👍 .

Ready for another pass, thanks! Hopefully we can get this merged after this pass :crossed_fingers: 😃

folletto commented 6 years ago

Checked on Nexus 5:

hypest commented 6 years ago

Hey @theck13 , there've been some code changes recently so, ready for another pass, thanks!

mattmiklic commented 6 years ago

All screens in the prologue look great, @hypest. I think it's ready to 🚢!

hypest commented 6 years ago

I think I addressed your comments @theck13 , ready for another pass. Thanks!

hypest commented 6 years ago

Ready for another pass @theck13 , thanks!