Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post-Signup Interstitial: Create interstitial screen #10933

Conversation

renanferrari
Copy link
Member

@renanferrari renanferrari commented Dec 10, 2019

Fixes #10923

Screens

Standard

Dark Mode

The dark mode layout has been scraped for now, as it depends on the work being done at the material-theme branch.

Landscape

Tablet

Notes

There are some slight differences between the layouts implemented here and the ones in the design spec (mainly the status bar color). Most of these differences are due to the fact that I'm reusing existing theme and styles instead of creating new ones just for this screen. It is my understanding that there is an ongoing effort to update the theme of the app as a whole, so I still need to make sure what would be the best approach here.

To test

To be able to test this, you need to:

  • Build and launch a debug version of the app.
  • Use adb to start the Activity when needed:
    adb shell am start -n org.wordpress.android/org.wordpress.android.ui.accounts.PostSignupInterstitialActivity

Standard layout

  1. Start the PostSignupInterstitialActivity on a phone in portrait mode.
  2. Verify that the layout is compliant with the design spec on Post-Signup Interstitial: Create interstitial screen #10923.

Landscape layout

  1. Start the PostSignupInterstitialActivity on a phone in landscape mode.
  2. Verify that the layout is compliant with the design spec on Post-Signup Interstitial: Create interstitial screen #10923.

Tablet layout

  1. Start the PostSignupInterstitialActivity on a tablet.
  2. Verifiy both texts and buttons don't expand horizontally more than what's been defined in the design spec on Post-Signup Interstitial: Create interstitial screen #10923.
  3. Rotate the tablet.
  4. Verify that the layout doesn't change.

Create new site button

  1. Sign in to an account without any sites.
  2. Start the PostSignupInterstitialActivity while WPMainActivity is on the back stack (more details on this below).
  3. Tap on the Create WordPress.com Site button.
  4. Verifiy that the SiteCreationActivity has been opened.
  5. Complete the site creation flow.
  6. Verify that the WPMainActivity is shown and the newly created site is selected.

Add self-hosted site button

  1. Sign in to an account without any sites.
  2. Start the PostSignupInterstitialActivity while WPMainActivity is on the back stack (more details on this below).
  3. Tap on the Add Self-Hosted Site button.
  4. Verifiy that the LoginActivity has been opened.
  5. Complete the site connection flow.
  6. Verify that the WPMainActivity is shown and the newly created site is selected.

Dismissal button

  1. Sign in to an account without any sites.
  2. Start the PostSignupInterstitialActivity.
  3. Tap on the Not Right Now button.
  4. Verify that the WPMainActivity has been opened on the Reader tab.

Notes

Please, check my comments below for more details on the assumptions I made while working on the buttons functionality and what is currently needed before moving forward.

@maxme maxme requested review from maxme, theck13 and shiki December 11, 2019 09:17
@maxme maxme added the Signup label Dec 11, 2019
@maxme maxme added this to the 13.9 milestone Dec 11, 2019
@SylvesterWilmott
Copy link

Looks great so far!

There are some slight differences between the layouts implemented here and the ones in the design spec (mainly the status bar color).

I am aware of this, the status bar colour is fine as is in the screenshots

We sometimes use a container width of 512dp max-width in some areas around the app. Can we try applying this constraint to the tablet layout (keeping the existing left and right paddings)?

@renanferrari
Copy link
Member Author

renanferrari commented Dec 11, 2019

Looks great so far!

Glad you like it!

We sometimes use a container width of 512dp max-width in some areas around the app. Can we try applying this constraint to the tablet layout (keeping the existing left and right paddings)?

If I understand correctly, that's something we can try for sure. I would just need to adjust this to 512dp.

Update: @SylvesterWilmott I have added the landscape layout and applied your suggestion regarding the max width.

@peril-wordpress-mobile
Copy link

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

Generated by 🚫 dangerJS

@renanferrari renanferrari force-pushed the issue/10923-create-interstitial-screen branch from 78cdc5e to 383fb60 Compare December 12, 2019 02:02
@SylvesterWilmott
Copy link

The changes look good to me! Thanks @renanferrari

Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

Having separate layouts is often a source of issues, I wish we could find a simple way to only have one.

PR looks good overall, I left minor comments.

I'll create a feature branch, so you can merge this one and target the same feature branch in the upcoming PRs.

@maxme maxme changed the base branch from develop to feature/10918-post-signup-interstitial December 12, 2019 14:45
@maxme
Copy link
Contributor

maxme commented Dec 12, 2019

I'll create a feature branch, so you can merge this one and target the same feature branch in the upcoming PRs.

Pushed a new branch: feature/10918-post-signup-interstitial

@renanferrari renanferrari changed the title [DO NOT MERGE] Post-Signup Interstitial: Create interstitial screen Post-Signup Interstitial: Create interstitial screen Dec 12, 2019
@renanferrari
Copy link
Member Author

Having separate layouts is often a source of issues, I wish we could find a simple way to only have one.

I agree. I have actually tried a few things, but none were quite good.

Our first iteration had a single layout, but the user had to scroll to see the buttons on a phone in landscape mode, which is not a good UX. @SylvesterWilmott fixed that with the new landscape layout.

From there, we experimented reusing the landscape layout on a tablet, but that didn't look good – and even if it did, we would still have at least two layouts: default and landscape.

The only alternatives I could think of that would let us achieve a single layout again would be to either update the ConstraintLayout lib to 2.0.0-beta2 so we could leverage its new Flow helper or to use the FlexboxLayout lib from Google. I just don't know if these alternatives can be considered "simple".

android:id="@+id/title_view"
android:layout_width="0dp"
android:layout_height="wrap_content"
android:layout_marginBottom="@dimen/margin_medium"
android:text="@string/post_signup_interstitial_title"
android:textAlignment="center"
android:textAppearance="@style/TextAppearance.AppCompat.Title"
app:fixWidowWords="true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Well spotted, 👍 I actually forgot about that one ;)

Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

LGTM, :shipit:

@maxme maxme merged commit 7e9e35d into wordpress-mobile:feature/10918-post-signup-interstitial Dec 13, 2019
@maxme
Copy link
Contributor

maxme commented Dec 19, 2019

There are some slight differences between the layouts implemented here and the ones in the design spec (mainly the status bar color). Most of these differences are due to the fact that I'm reusing existing theme and styles instead of creating new ones just for this screen. It is my understanding that there is an ongoing effort to update the theme of the app as a whole, so I still need to make sure what would be the best approach here.

Sorry, I just realized that, but I think we could reuse @style/WordPress.Button.Primary for the 2 primary buttons. I'll add a comment in the follow up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants