Skip to content

Conversation

@mkevins
Copy link
Contributor

@mkevins mkevins commented May 16, 2022

Fixes #16478

Description

This PR adds a bottom helper text to the theme picker screen by implementing an optional footer parameter to layouts recycle view. This is useful for working around scroll issues in the layout that houses the recycler view. It introduces a new abstraction LayoutRowViewHolder from which LayoutItemViewHolder and LayoutFooterViewHolder extend:

classDiagram
  class LayoutRowViewHolder
  <<Sealed>> LayoutRowViewHolder
  LayoutRowViewHolder <|-- LayoutItemViewHolder: Extends
  LayoutRowViewHolder <|-- LayoutFooterViewHolder: Extends
Loading

The footer is inflated when needed from the passed resource id (the overriden item count is incremented when not null).

It also adds the footer as a LayoutCategoryViewType so that it isn't mixed with the regular items during view recycling.

To test:

Theme picker

  1. Start the site creation flow
  2. Select an intent (or skip)
  3. Type a site name (or skip)
  4. Expect to see the theme picker screen
  5. Scroll to the bottom of the theme picker screen
  6. Expect to see the helper text in the footer at the bottom of the screen

Page picker

  1. Start the page picker flow (create a new page from the page list screen)
  2. Expect to see the page picker screen
  3. Scroll to the bottom of the page layout categories
  4. Expect that the footer is absent

Regression Notes

  1. Potential unintended areas of impact
    Page layout picker

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

  3. What automated tests I added (or what prevented me from doing so)
    Our tests are currently structured to test the view models, but these changes don't rely on any view model logic.

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 added 3 commits May 13, 2022 18:31
This implements an optional footer parameter to layouts recycle view,
useful for working around scroll issues in the screen where it is used.
It introduces a new abstraction `LayoutRowViewHolder` from which
`LayoutItemViewHolder` and `LayoutFooterViewHolder` extend.

The footer is inflated when needed from the passed resource id (the
overriden item count is incremented when not null).

It also adds the footer as a `LayoutCategoryViewType` so that it isn't
mixed with the regular items during view recycling.
…vamp--add-helper-text-to-bottom-as-list-item
@mkevins mkevins added Site Creation Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. labels May 16, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 16, 2022

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@mkevins mkevins requested review from antonis and ovitrif May 16, 2022 01:34
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 16, 2022

You can test the changes on this Pull Request by downloading the APKs:

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!

The solution is elegant imho and works as expected 👍

I did notice two small issues:

  1. Sometimes the Choose a theme screen starts scrolled to the bottom of the recycler view (this happens only when the device is proxied to use a wpcom sandbox with the pending api changes active):
testpreview.mp4
  1. The style of the footer text might need a few adjustments to match the i3 design:
Expected Actual
expected actual

I was thinking that achieving this might be easier with reusing an existing style after we merge #16556.

@ovitrif ovitrif linked an issue May 16, 2022 that may be closed by this pull request
mkevins added 3 commits May 17, 2022 13:03
…vamp--add-helper-text-to-bottom-as-list-item
This prevents an issue where the fetched categories arriving later after
the layout caused the list to be scrolled to the bottom (maintaining a
view of the footer as the new items were added).
@wpmobilebot
Copy link
Contributor

Found 1 violations:

The PR caused the following dependency changes:

-+--- org.wordpress:fluxc:{strictly trunk-b1d4cb198c9c57cd80594171227dae5c96e58d56} -> trunk-b1d4cb198c9c57cd80594171227dae5c96e58d56
-|    +--- org.wordpress:wellsql:1.7.0
-|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
-|    +--- org.wordpress.fluxc:fluxc-annotations:trunk-b1d4cb198c9c57cd80594171227dae5c96e58d56
-|    +--- org.greenrobot:eventbus:3.3.1 (*)
-|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
-|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
-|    +--- androidx.paging:paging-runtime:2.1.2
-|    |    +--- androidx.paging:paging-common:2.1.2
-|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
-|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
-|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.3.1 (*)
-|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.2.0 (*)
-|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- com.goterl:lazysodium-android:5.0.2
-|    +--- net.java.dev.jna:jna:5.5.0
-|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
-|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
-|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
-|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
-|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.1.0-beta01 (*)
-|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
-|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
-|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
-|    +--- com.google.code.gson:gson:2.8.5
-|    +--- org.apache.commons:commons-text:1.1 (*)
-|    +--- androidx.room:room-runtime:2.4.2 (*)
-|    +--- androidx.room:room-ktx:2.4.2
-|    |    +--- androidx.room:room-common:2.4.2 (*)
-|    |    +--- androidx.room:room-runtime:2.4.2 (*)
-|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
-|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
-|    +--- com.google.dagger:dagger:2.29.1 -> 2.41 (*)
-|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
-|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
++--- org.wordpress:fluxc:{strictly 1.42.0} -> 1.42.0
+|    +--- org.wordpress:wellsql:1.7.0
+|    |    \--- org.wordpress.wellsql:wellsql-annotations:1.7.0
+|    +--- org.wordpress.fluxc:fluxc-annotations:1.42.0
+|    +--- org.greenrobot:eventbus:3.3.1 (*)
+|    +--- com.squareup.okhttp3:okhttp:4.9.0 -> 4.9.2 (*)
+|    +--- com.android.volley:volley:1.1.1 -> 1.2.0
+|    +--- androidx.paging:paging-runtime:2.1.2
+|    |    +--- androidx.paging:paging-common:2.1.2
+|    |    |    +--- androidx.annotation:annotation:1.0.0 -> 1.2.0
+|    |    |    \--- androidx.arch.core:core-common:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.arch.core:core-runtime:2.0.0 -> 2.1.0 (*)
+|    |    +--- androidx.lifecycle:lifecycle-runtime:2.0.0 -> 2.3.1 (*)
+|    |    +--- androidx.lifecycle:lifecycle-livedata:2.0.0 -> 2.2.0 (*)
+|    |    \--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- com.goterl:lazysodium-android:5.0.2
+|    +--- net.java.dev.jna:jna:5.5.0
+|    +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.10 (*)
+|    +--- org.jetbrains.kotlin:kotlin-android-extensions-runtime:1.6.10 (*)
+|    +--- androidx.appcompat:appcompat:1.0.2 -> 1.3.1 (*)
+|    +--- androidx.recyclerview:recyclerview:1.0.0 -> 1.1.0 (*)
+|    +--- androidx.exifinterface:exifinterface:1.0.0 -> 1.1.0-beta01 (*)
+|    +--- com.squareup.okhttp3:okhttp-urlconnection:4.9.0 -> 4.9.2
+|    |    +--- com.squareup.okhttp3:okhttp:4.9.2 (*)
+|    |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.4.10 -> 1.6.10 (*)
+|    +--- com.google.code.gson:gson:2.8.5
+|    +--- org.apache.commons:commons-text:1.1 (*)
+|    +--- androidx.room:room-runtime:2.4.2 (*)
+|    +--- androidx.room:room-ktx:2.4.2
+|    |    +--- androidx.room:room-common:2.4.2 (*)
+|    |    +--- androidx.room:room-runtime:2.4.2 (*)
+|    |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.10 (*)
+|    |    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.5.2 (*)
+|    +--- com.google.dagger:dagger:2.29.1 -> 2.41 (*)
+|    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.3.9 -> 1.5.2 (*)
+|    \--- org.jetbrains.kotlinx:kotlinx-coroutines-android:1.3.9 -> 1.5.2 (*)
 \--- org.wordpress:login:0.14.0
-     \--- org.wordpress:fluxc:trunk-1436f46fc296ede0e65e117bbfced38fa34cec6d -> trunk-b1d4cb198c9c57cd80594171227dae5c96e58d56 (*)
+     \--- org.wordpress:fluxc:trunk-1436f46fc296ede0e65e117bbfced38fa34cec6d -> 1.42.0 (*)

Please review and act accordingly

@mkevins
Copy link
Contributor Author

mkevins commented May 17, 2022

Thanks for reviewing and testing Ovi, and for the great feedback!

I was thinking that achieving this might be easier with reusing an existing style after we merge #16556.

Spot on. I was hoping to consolidate some of this to be less ad-hoc, and the PR mentioned allows that. I updated to use that, and adjusted a few other things (01d5a85) (e.g. the icon was dark in dark mode, and that should be fixed now).

Sometimes the Choose a theme screen starts scrolled to the bottom of the recycler view (this happens only when the device is proxied to use a wpcom sandbox with the pending api changes active):

I observed this too, even without sandboxing. I believe what is happening is that the footer is displayed without any items, and when the fetch response comes back, the items are added, but the recycler view maintains the scroll position to be aligned with the footer. I pushed a change (3812665) which should fix this by conditionally displaying the footer, only when there are items.

This is ready for another pass. 👍

@ovitrif
Copy link
Contributor

ovitrif commented May 17, 2022

Thanks for reviewing and testing Ovi, and for the great feedback!

I was thinking that achieving this might be easier with reusing an existing style after we merge #16556.

Spot on. I was hoping to consolidate some of this to be less ad-hoc, and the PR mentioned allows that. I updated to use that, and adjusted a few other things (01d5a85) (e.g. the icon was dark in dark mode, and that should be fixed now).

Sometimes the Choose a theme screen starts scrolled to the bottom of the recycler view (this happens only when the device is proxied to use a wpcom sandbox with the pending api changes active):

I observed this too, even without sandboxing. I believe what is happening is that the footer is displayed without any items, and when the fetch response comes back, the items are added, but the recycler view maintains the scroll position to be aligned with the footer. I pushed a change (3812665) which should fix this by conditionally displaying the footer, only when there are items.

This is ready for another pass. 👍

I gave this another pass and I feel it's ready to be merged:

  • the issue when the list was scrolled to the bottom (with the helper text visible) didn't reproduce
  • the design looks good and conforms to the design
  • Different variations -- dark mode, tablet, landscape -- look nice

Thank you for your work on this @mkevins 🙇

@ovitrif ovitrif merged commit 1509790 into feature/site-design-revamp May 17, 2022
@ovitrif ovitrif deleted the feature/site-design-revamp--add-helper-text-to-bottom-as-list-item branch May 17, 2022 08:34
@antonis antonis mentioned this pull request May 24, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Part of a WIP Feature This label is used to disable milestone checks for PRs that are not against `develop` or `release`. Site Creation [Type] Task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Site Design Revamp] Main View - Add helper text to bottom

4 participants