Skip to content

Conversation

@mkevins
Copy link
Contributor

@mkevins mkevins commented Apr 27, 2022

This PR addresses parts A, B, and C of #16392

Primarily, this PR removes the thumbnail viewport mode selector in the site design screen. Additionally, since the changes are fairly trivial, this also removes the screen's subtitle text and changes the wording of the title.


Note: We may want to also consider updating this test:

fun `when the picker starts on a tablet the tablet thumbnails or preview load by default`() = mockResponse {
viewModel.start(isTablet = true)
val captor = ArgumentCaptor.forClass(PreviewMode::class.java)
verify(previewModeObserver).onChanged(captor.capture())
assertThat(requireNotNull(captor.value as PreviewMode)).isEqualTo(PreviewMode.TABLET)
}

if we decide to default to PreviewMode.MOBILE even for tablets, since on the main screen we will no longer have a way to change them. Also, it should be considered whether or not we want the thumbnail preview modes to change when the user comes back from the preview, since this could be confusing with no way to change the mode on that screen. 🤔

To test:

Theme (Site Design) Picker

  1. Start the site creation flow
  2. Choose an Intent (or skip)
  3. Choose a site name (or skip)
  4. Expect to be at the site design screen

Expectations:

  • The title should be "Choose a theme"
  • The PreviewMode selector should be gone
  • The subtitle text should be gone

Page Picker

  1. Navigate to the pages list
  2. Tap the FAB to create a new page
  3. Expect to be at the page layout picker

Expectations:

  • The PreviewMode selector should still be present (and working as usual)
  • The subtitle text should still be present

Regression Notes

  1. Potential unintended areas of impact
    Modal layout picker (for page creation).

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manually tested this to ensure that there are no regressions. Also, there are automated unit tests for the underlying view model.

  3. What automated tests I added (or what prevented me from doing so)
    The view models for the shared implementations have unit tests, but I've added a note to consider changing an existing view model test, pending further decisions on the desired UX.

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 6 commits April 27, 2022 07:54
This isn't needed for this task, but is useful to satisfy the IDE :)
Since we are removing some parts of an implementation that shares a base
class with the page picker, there will be some changes needed to adapt
these for the new flow. The intent with these comments is to serve as a
place-holder denoting some things that need to be rewired.
@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 Apr 27, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 27, 2022

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

…tor' into feature/site-design-revamp--remove-bottom-toolbar
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 27, 2022

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

@mkevins mkevins requested review from antonis and ovitrif April 27, 2022 07:01
@mkevins mkevins marked this pull request as ready for review April 27, 2022 07:01
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.

Everything worked as expected in my tests 🎉

Thank you @mkevins for your work, it lgtm and the only thing we may have to do before merging this is to remove the hpp_subtitle string resource.

@twstokes
Copy link
Contributor

👋 I've created some testing steps in the WPiOS equivalent of this PR that may be handy for Android.

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.

I've tested the updates on a phone and tablet emulator with the testing steps and it works as expected.

One small issue I noticed which I'm not sure it's desired is that there is no title for the screen on tablet:

tablet

This seems to be an issue on all the new screens we added recently:

Site Intent Question Site Name
siq_tablet sn_tablet

Because of this I created a ticket on our internal backlog to fix the issue for every screen.

…evamp--remove-bottom-toolbar

[Site design revamp] Remove bottom toolbar
@ovitrif
Copy link
Contributor

ovitrif commented Apr 28, 2022

Feels ready to be merged, I'll do that after the CI checks pass. 🚀 🚀 🚀

@ovitrif
Copy link
Contributor

ovitrif commented Apr 28, 2022

Android Lint failed because there was another resource left unused that we missed to remove.

I pushed the change to this PR since it's trivial and it's avoidable overhead to actually create a PR just for this.

Will merge it if the checks are green this time 🤞

@ovitrif ovitrif merged commit 7dbe9ed into feature/site-design-revamp Apr 28, 2022
@ovitrif ovitrif deleted the feature/site-design-revamp--remove-thumbnail-mode-selector branch April 28, 2022 12:25
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants