Skip to content

Conversation

@mkevins
Copy link
Contributor

@mkevins mkevins commented Aug 30, 2022

Fixes #17072

Description

This PR implements the WordPress landing screen redesign behind the respective feature flag. The approach is to create a revamped fragment to co-exist with the current implementation, and use this new fragment conditionally (based on the flag) from the login activity. When the feature flag is turned on by default, the older flow can be removed.

This design is implemented via Jetpack Compose, and a few things are not complete. To keep scope minimal for each PR, we can leverage the feature flag to develop partially complete UI with more granular feedback.

Since toggling the feature flag requires login, and navigating to this screen requires being logged out, it can be cumbersome to go back and forth between implementations. To make this easier, it is useful to apply the following patch:

Hard-coded flag patch
diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java
index df7d849e4d..fccaf5aa14 100644
--- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java
+++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java
@@ -227,7 +227,7 @@ public class LoginActivity extends LocaleAwareActivity implements ConnectionCall
     }
 
     private void loginFromPrologue() {
-        if (mLandingScreenRevampFeatureConfig.isEnabled()) {
+        if (true) {
             showFragment(new LoginPrologueRevampedFragment(), LoginPrologueRevampedFragment.TAG);
         } else {
             showFragment(new LoginPrologueFragment(), LoginPrologueFragment.TAG);
Former Solved Details

I also wired up one of the buttons to the login flow, because without the above hack, it isn't possible to toggle the feature flag off once logged out otherwise. The button functionality is scoped for a later PR, so this is an exception for practicality.

Update: Both login buttons were updated to have the expected functionality.

Questions

While this PR is still in draft, there are a couple of questions worth discussing before finalizing it:

  1. The added font resource contributes significant size to the apk (~744k), but we only use this font in this one screen. Do we plan to use this font else-where, or is this considered acceptable?
    • There is a possibility to use "Downloadable fonts" for this, reducing the apk size, but this raises other questions, such as how to support users on older Android versions where this feature is not supported, etc.
  2. The brush stroke image was rasterized directly from Figma as a 4x png. This seems to look ok in my testing, but it would be great to see how it looks on various form-factors. In the designs, the brush stroke is on the right side of the screen, but in this implementation, for RTL locales, it is on the left side of the screen. Is this ok?
    • Also, the image in RTL is not horizontally flipped. Since it is just a vertical brush stroke, this seemed ok to me, but it would be great to hear some other opinions.

Note: The text for these buttons is based on future designs, and may possibly be reverted for the first iteration. I have deferred this decision for now.

Update: This has been reverted back to the former texts.

Screenshots - Login Prologue

Light Dark
wordpress-prologue wordpress-prologue-dark
RTL Landscape
wordpress-prologue-rtl wordpress-prologue-landscape

To test:

Precondition: Enable the feature flag (either hard-coding via the patch above, or toggling within the app - or both)

  1. Logout of the app once the feature flag is enabled
  2. The new login screen should be visible
  3. Put the device in dark mode
  4. The dark mode design should be readable
  5. Change to an RTL locale
  6. Change the device orientation
  7. Expect each view to be show as above

Login:

  1. From the login screen, tap the "Log in or sign up with WordPress.com" button
  2. Expect to be presented with the login flow
  3. Go back
  4. Tap the "Enter your existing site address" button
  5. Expect to be presented with the site address login flow

Precondition: Disable the feature flag

  1. Logout of the app
  2. The old screen should be visible

Jetpack:

The Jetpack flavor of the app should be unaffected by these changes.

  1. Build (or install from this PR) the Jetpack flavor of the app
  2. Logout (with the feature enabled or disabled - it shouldn't matter)
  3. The login screen should be unaffected

Regression Notes

  1. Potential unintended areas of impact
    WordPress and Jetpack login screen

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)
    This PR only adds UI changes (under a feature flag).

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@mkevins mkevins added this to the 20.7 milestone Aug 30, 2022
@mkevins mkevins requested a review from ovitrif August 30, 2022 20:25
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 30, 2022

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr17104-5da1473.apk), or scanning this QR code:

Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work on this @mkevins.

It's lovely to see Compose work on the project, kudos for stepping forward, doing this and suggesting it for myself too as I'm working on the JP landing screen revamp 🙇 .

The code LGTM, I have a few comments that are part of the review but nothing big.

There's 3 more things I've been thinking of and wanted to share:

  1. For some reason, when tapping the back button on the screen that comes after tapping the "Connect to your WordPress site" it gets me out of the app.
  2. I was thinking if we could place the bg brush stroke in Landscape not to the end edge, but rather at some specific percentile offset. I like how it appears behind the other UI elements, but in landscape orientation it feels like it's just there somewhere in the background. Wdyt?
  3. Are you planning to make the view "full screen"? It feels like it gives a more immersive experience imho when looking at the designs. Those UI bars are cutting stuff 😢 and removing that feeling a bit. My prefs goes for the original design with the full-screen experience:

Screenshot 2022-08-31 at 17 18 32

@mkevins
Copy link
Contributor Author

mkevins commented Aug 31, 2022

  • For some reason, when tapping the back button on the screen that comes after tapping the "Connect to your WordPress site" it gets me out of the app.

This is actually the existing behavior.. I haven't looked into why this is happening, but if I can find the root cause, I'll try to see if it's in scope to address it in this revamp.

  • I was thinking if we could place the bg brush stroke in Landscape not to the end edge, but rather at some specific percentile offset. I like how it appears behind the other UI elements, but in landscape orientation it feels like it's just there somewhere in the background. Wdyt?

I'm not sure I entirely follow what you mean here.. but I guess you mean to move the brush stroke closer to the middle? It can certainly be achieved with some spacer and a percentage - this should be easy to change if so. I'll defer this until the question about the RTL brush stroke is resolved, since it may be something that can be tackled together.

  • Are you planning to make the view "full screen"? It feels like it gives a more immersive experience imho when looking at the designs. Those UI bars are cutting stuff 😢 and removing that feeling a bit. My prefs goes for the original design with the full-screen experience:

I'll try with fullscreen, good point!

@ovitrif
Copy link
Contributor

ovitrif commented Aug 31, 2022

I'm not sure I entirely follow what you mean here.. but I guess you mean to move the brush stroke closer to the middle? It can certainly be achieved with some spacer and a percentage - this should be easy to change if so. I'll defer this until the question about the RTL brush stroke is resolved, since it may be something that can be tackled together.

It was indeed what you're describing here 👍
Good idea with deferring until we see what feedback we get on the RTL brush stroke 🙇

@ovitrif
Copy link
Contributor

ovitrif commented Sep 7, 2022

  • For some reason, when tapping the back button on the screen that comes after tapping the "Connect to your WordPress site" it gets me out of the app.

This is actually the existing behavior.. I haven't looked into why this is happening, but if I can find the root cause, I'll try to see if it's in scope to address it in this revamp.

I found the root cause of this issue and added a fix on my PR. See comment: #17117 (comment)

@mkevins Can you please add the same change to this PR, and perhaps target the feature branch feature/revamp-landing-screen since my PR is supposed to be merged after yours according to its Merging Strategy.

Thanks in advance 🙇

@mkevins mkevins changed the base branch from trunk to feature/revamp-landing-screen September 7, 2022 15:41
@mkevins
Copy link
Contributor Author

mkevins commented Sep 7, 2022

Can you please add the same change to this PR, and perhaps target the feature branch feature/revamp-landing-screen since my PR is supposed to be merged after yours according to its Merging Strategy.

Thanks in advance

Good idea Ovi! I've changed the merge target, and added the crash fix to this PR as well. Also, I've updated the feature branch to be up to date with trunk. This is ready for another pass.

@mkevins mkevins marked this pull request as ready for review September 7, 2022 15:44
@mkevins mkevins requested a review from ovitrif September 7, 2022 15:44
Copy link
Contributor

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @mkevins 👏

Thank you for wrangling this 👍

Everything works as described, I only found two small issues.

  1. The appbar appears behind the transparent statusbar on the Connect to your WordPress site screen:

    I presume the same fix that was done here would work on this PR. That is moving the code that clears the FLAG_LAYOUT_NO_LIMITS flag to onPause.
    More info in this comment.

  2. I noticed a rather unwanted effect when tapping the transparent Looking to create a new site? button.

Preview:

transparent_button_issue.mp4

@antonis antonis self-requested a review September 8, 2022 15:02
@antonis antonis modified the milestones: 20.8, 20.9 Sep 15, 2022
@ovitrif ovitrif self-assigned this Sep 19, 2022
@ovitrif
Copy link
Contributor

ovitrif commented Sep 19, 2022

I'll be continuing on the great work started by Matthew to get this PR ready, as discussed via dm 🙇

Matthew Kevins added 9 commits September 19, 2022 20:09
Note: it is possible that this will need to be optimized.
This will likely need some modifications. Initially, I attempted to
import the vector image for the brush stroke, but the paths were too
long, and could not be parsed during compilation. I rasterize them in
this commit, but this may not work well in various screen sizes /
orientations.
mkevins and others added 13 commits September 19, 2022 20:09
Note: this adds a significant size to the apk (~744k), so it is possible
that we may opt to forgo this. If so, this branch can be rebased to
remove this from the repo history as well (instead of reverting).
This helps prevent bloating the Jetpack app with unused resources.
This is to avoid having the brushstroke visible beneath the icon in
scenarios where it overlaps. This also sets a circle clip shape for the
icon so that the background color doesn't create a rectangular artifact
on top of the brush stroke background.
Using a Canvas seems to be necessary here to avoid "pre-clipping" the
image to the container size, which would otherwise leave a small gap
when offsetting by a negative value on the y-axis.
This seems better than using arbitrarily chosen dimensions.
Copy link
Contributor

@antonis antonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested on an older Huawei P9 (Android 7.0) and get the following error while starting up the WordPress app:

java.lang.IllegalStateException: Cannot create Typeface from ResourceFont(resId=2131296256, weight=FontWeight(weight=400),

(full stack trace).
I'll investigate further and report back.

Edit:
Commenting out the custom font assignment prevents the crash. My guess is that this is a memory or compose issue on older devices.

The brush does not load at all on the background though. I'm not sure yet if this is due to the screen size or the resource fails to load silently 🤔

@ovitrif ovitrif force-pushed the issue/17072-remove-carousel-and-implement-wordpress-landing-screen-redesign branch from 2e5eb57 to b6803e2 Compare September 20, 2022 16:12
@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

Copy link
Contributor

@antonis antonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @mkevins and @ovitrif 👍
I've retested with the latest changes both on my Pixel 5 (Android 13) and my Huawei P9 (Android 7) and the issues reported before are fixed. The code LGTM too 🎉
As discussed the CI check failures will be handled separately from another PR also pointing on feature/revamp-landing-screen

@antonis antonis merged commit fa5948c into feature/revamp-landing-screen Sep 21, 2022
@antonis antonis deleted the issue/17072-remove-carousel-and-implement-wordpress-landing-screen-redesign branch September 21, 2022 12:54
@mkevins
Copy link
Contributor Author

mkevins commented Sep 26, 2022

Thank you for addressing the remaining issues on this one Ovi! And thanks for testing and reviewing Antonis! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Revamp Landing Screen] WPAndroid - Remove carousel & redesign landing screen

6 participants