Skip to content

Conversation

@carl-duncan
Copy link
Contributor

@carl-duncan carl-duncan commented May 31, 2022

Fixes #

Fixed the category titles (and subtitles) displayed in RTL languages for the theme picker and the page picker.

To test:

Set the app's language to an RTL language. (pseudoLocales can be used for ease in testing).

LTR

Make sure there are no regression for the theme picker and the page picker.

  1. Enable LTR locale (U.S. English)
  2. Start site creation flow
  3. Choose an intent (e.g. Food)
  4. Expect to see the theme picker screen next
  5. Make sure category titles (and subtitles) are displayed correctly
    • There should be no changes from this PR
  6. Exit the theme picker and go to the pages list
  7. Tap the FAB to create a new page
  8. Expect to see the page picker screen next
  9. Make sure category titles (and subtitles) are displayed correctly
    • There should be no changes from this PR

RTL

Make sure there are no regression for the theme picker and the page picker.

  1. Enable RTL locale (Arabic, English pseudoLocale)
  2. Start site creation flow
  3. Choose an intent (e.g. Food)
  4. Expect to see the theme picker screen next
  5. Make sure category titles (and subtitles) are displayed correctly
    • Categories should be on the right side with proper padding
  6. Exit the theme picker and go to the pages list
  7. Tap the FAB to create a new page
  8. Expect to see the page picker screen next
  9. Make sure category titles (and subtitles) are displayed correctly
    • Categories should be on the right side with proper padding

Regression Notes

  1. Potential unintended areas of impact
    Page and theme pickers

  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)
    Out of scope for this task.

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.

Copy link
Contributor

@mkevins mkevins 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 tackling this issue, it looks like it is off to a good start. I've tested this, and the categories are now displayed on the correct side in RTL locales.

I noticed a minor change (loss in padding) around the category labels:

rtl-padding-diff

@carl-duncan
Copy link
Contributor Author

Thank you for tackling this issue, it looks like it is off to a good start. I've tested this, and the categories are now displayed on the correct side in RTL locales.

I noticed a minor change (loss in padding) around the category labels:

rtl-padding-diff rtl-padding-diff

Fixed

@antonis antonis requested a review from mkevins June 3, 2022 06:16
@mkevins
Copy link
Contributor

mkevins commented Jun 9, 2022

Hi @carl-duncan 👋 😃

There are some changes that have landed in trunk that touch the same file that this PR touches. It appears that there are no conflicts from git when merging these, however, testing the merged result reveals that something may have been affected by this. To test this, I checked out this PR locally, and brought it up to date by merging in the latest trunk (which includes the changes brought in by this PR: #16468). Though the conflicting changes were auto-merged by git, testing the code I encountered more issues with padding again 😞. The biggest difference was noticeable in the site creation flow (theme picker), with a more subtle padding shift in the page layout picker:

Site Creation Theme Picker Page Layout Picker
categories-padding-diff pages-padding-diff

Can you try merging in the latest trunk and see if you see the same?

@carl-duncan
Copy link
Contributor Author

carl-duncan commented Jun 13, 2022

Hi @carl-duncan 👋 😃

There are some changes that have landed in trunk that touch the same file that this PR touches. It appears that there are no conflicts from git when merging these, however, testing the merged result reveals that something may have been affected by this. To test this, I checked out this PR locally, and brought it up to date by merging in the latest trunk (which includes the changes brought in by this PR: #16468). Though the conflicting changes were auto-merged by git, testing the code I encountered more issues with padding again 😞. The biggest difference was noticeable in the site creation flow (theme picker), with a more subtle padding shift in the page layout picker:

Site Creation Theme Picker Page Layout Picker
categories-padding-diff
pages-padding-diff
Can you try merging in the latest trunk and see if you see the same?

I will sort this out later today, thank you for the review!

@carl-duncan carl-duncan force-pushed the fix/16456-template-rtl-incorrectly-displayed branch from 8230d81 to 0b754c2 Compare June 18, 2022 16:06
@carl-duncan carl-duncan reopened this Jun 18, 2022
@carl-duncan
Copy link
Contributor Author

Good day, I am sorry about the delays, here is the updates! I updated the padding for the end so that in RTL displays it will match exactly

Copy link
Contributor

@mkevins mkevins 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 these changes @carl-duncan ! I've tested the latest changes, and this indeed improves the padding for RTL locales, while leaving the LTR intact. I've tested this with both the theme picker and the page picker, and things are working well. I've left one nitpick comment about.

@carl-duncan carl-duncan force-pushed the fix/16456-template-rtl-incorrectly-displayed branch from 3412528 to a0f93d5 Compare June 21, 2022 12:34
@carl-duncan carl-duncan force-pushed the fix/16456-template-rtl-incorrectly-displayed branch from a0f93d5 to 37dd21e Compare June 21, 2022 12:36
@mkevins mkevins self-requested a review June 21, 2022 20:05
Copy link
Contributor

@mkevins mkevins 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 Carl, the padding is now fixed, and the code is looking good! Compared with trunk, the LTR locales are unaffected (i.e. no regressions) and the padding for RTL locales is improved:

Screen trunk This PR
Theme Picker theme-picker-rtl-trunk theme-picker-rtl-duncan
Page Picker page-picker-rtl-trunk page-picker-rtl-duncan

I've added two small suggestions for improving this further, so that we can have correct alignment for RTL locales in addition to the improved padding. The results look like this:

Screen This PR This PR with suggestions applied
Theme Picker theme-picker-rtl-duncan theme-picker-rtl-match-parent
Page Picker page-picker-rtl-duncan page-picker-rtl-match-parent

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jun 23, 2022

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

Copy link
Contributor

@mkevins mkevins 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 updating Carl. I've updated the description of the PR to add the testing steps (already performed) for posterity. I also added some labels and set the milestone to the next release. I've created a companion branch in the main repository to trigger CI tests, and once green, this one should be good to go. Thanks for your efforts on this!

@mkevins mkevins merged commit d14f1a1 into wordpress-mobile:trunk Jun 23, 2022
@AliSoftware
Copy link
Contributor

AliSoftware commented Jun 27, 2022

Thanks again for this PR @carl-duncan 🎉

For information, your fix has been included in the version 20.2-rc-1 that we are now sending to beta-testing phase before releasing it to the public in two weeks. Be sure to watch https://make.wordpress.org/mobile/ for a soon-to-be published post about the beta-testing of WPAndroid 20.2, to help us test this version and your fix that shipped in it!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants