Skip to content

Conversation

@ovitrif
Copy link
Contributor

@ovitrif ovitrif commented Apr 27, 2022

Fixes point D on #16392.

Removes the category filter pills on the Site design theme screen & sets the text alignment of the large "Choose a theme" title to the left.

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 horizontal scrolling category filter pills are gone.
  • The text of the large title is aligned at the left (right for RTL languages).

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 horizontal scrolling category filter pills are not changed.
  • The text of the large title is still aligned at the center of the screen.

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)
    Manual testing to ensure there's no regression on that screen.

  3. What automated tests I added (or what prevented me from doing so)
    Existing view models unit tests.

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.

@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.

@ovitrif ovitrif requested a review from mkevins April 27, 2022 19:26
@ovitrif ovitrif 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
@ovitrif ovitrif changed the base branch from trunk to feature/site-design-revamp April 27, 2022 19:26
@ovitrif ovitrif requested a review from a team April 27, 2022 19:31
@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:

@ovitrif
Copy link
Contributor Author

ovitrif commented Apr 28, 2022

At the moment we're waiting for feedback related to an internal comment pbArwn-4rY-p2#comment-5630, otherwise this PR is ready, not a draft.

@ovitrif ovitrif marked this pull request as ready for review April 29, 2022 10:04
@ovitrif ovitrif force-pushed the feature/site-design-revamp--remove-category-filter-pills branch from 2f4c539 to 69b4968 Compare April 29, 2022 10:06
@twstokes
Copy link
Contributor

I tested with the latest build:

  • ✅ The horizontal scrolling category filter pills were gone for the theme screen
  • ✅ The large title was left aligned for LTR and right aligned for RTL languages for the theme screen
  • ✅ The page layout picker still had the horizontally scrolling category filter pills
  • ✅ The page layout picker's title was center aligned

Great work! 🎉

Two things I noticed:

  1. When in landscape mode, should we still keep the collapsed title text center aligned?

  1. When I was using Hebrew to test RTL languages I noticed that the thumbnails images weren't centered. I looked, but didn't see an existing WPAndroid issue for this.

@twstokes
Copy link
Contributor

  1. When in landscape mode, should we still keep the collapsed title text center aligned?

I've opened up a question about this to confirm. I don't it's a blocker for this PR.

Internal ref: pbArwn-4rY-p2#comment-5693

@ovitrif
Copy link
Contributor Author

ovitrif commented May 2, 2022

Great work! 🎉

Two things I noticed:

  1. When in landscape mode, should we still keep the collapsed title text center aligned?
  2. When I was using Hebrew to test RTL languages I noticed that the thumbnails images weren't centered. I looked, but didn't see an existing WPAndroid issue for this.

Thank you @twstokes for the review and for reporting the two issues.

  1. For the collapsed title I'll look into fixing it.
  2. For the thumbnails not being centered issue, I'll create a WPAndroid issue for it.

@ovitrif
Copy link
Contributor Author

ovitrif commented May 2, 2022

@twstokes I created a new issue for the bug that you reported: #16456 and I pushed the commits to align the collapsed title text to the left (textStart to work with RTL).

@twstokes
Copy link
Contributor

twstokes commented May 2, 2022

@twstokes I created a new issue for the bug that you reported: #16456 and I pushed the commits to align the collapsed title text to the left (textStart to work with RTL).

Thank you @ovitrif!

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

Hey @ovitrif! 👋

The collapsed title was left-aligned as expected with the recent changes. When I changed to a RTL language (Hebrew), though, the title disappeared. Is this supposed to happen?

LTR RTL
Screenshot_20220502-123847 Screenshot_20220502-124328

@ovitrif
Copy link
Contributor Author

ovitrif commented May 2, 2022

Hey @ovitrif! 👋

The collapsed title was left-aligned as expected with the recent changes. When I changed to a RTL language (Hebrew), though, the title disappeared. Is this supposed to happen?

Definitely not, thanks for catching this 🙇 .

I'll test it a bit more. Something's off with the titles on the site creation flow screens as they disappear on tablet, but this problem that you just reported might be different. I did notice it a few times though not regularly as in this case 👍

@ovitrif
Copy link
Contributor Author

ovitrif commented May 2, 2022

@twstokes Is it also the same for you that in RTL languages (Hebrew) on landscape the titles for all the new Site Creation screens are not shown?

It seems to be the case for me:

Site Intent Question Site Name Site Theme
intent name theme

If that's true I'll create another issue on our board, I added one last week about the titles not being show on tablets 😓

@twstokes
Copy link
Contributor

twstokes commented May 2, 2022

@twstokes Is it also the same for you that in RTL languages (Hebrew) on landscape the titles for all the new Site Creation screens are not shown?

Good call @ovitrif - my device matches this behavior as well. A separate issue sounds good to me, so this one is good to merge! 🚀

@ovitrif
Copy link
Contributor Author

ovitrif commented May 3, 2022

@twstokes I created an issue for WP-Android regarding the missing titles in RTL on Landscape: #16465.

Gotta merge this PR once the CI checks are green (flaky unit test already fixed on trunk is causing us troubles).

CI is now green 🚀

@ovitrif ovitrif merged commit 02bccd2 into feature/site-design-revamp May 3, 2022
@ovitrif ovitrif deleted the feature/site-design-revamp--remove-category-filter-pills branch May 3, 2022 09:06
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.

3 participants