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

Introduce updated locale picker #16331

Merged
merged 35 commits into from
Apr 28, 2022
Merged

Introduce updated locale picker #16331

merged 35 commits into from
Apr 28, 2022

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Apr 14, 2022

Note, this PR was originally created by @khaykov at #14836. As the develop branch was deleted, it was not possible to reopen the original PR, which is why a new PR has been generated based on trunk.

This PR updates the look of the app's locale picker, following the same design as the site timezone picker. It can be searched by both label and localized label.

Image from Gyazo

To test:

  • Navigate to App Settings, and open the Interface Language option.
  • Confirm that the new locale picker works as expected:
    - You can filter locales by both their label and localized label (eg. typing "Dani" and "Dans" should produce list with "Danish/Dansk" locale)
    - Tapping on locale changes app language.
    - Current app locale is displayed a the top of the list. Tapping on it closes the list.

Regression Notes

  1. Potential unintended areas of impact
  • Locale Picker is not working!
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Manual testing + VM tests.
  1. What automated tests I added (or what prevented me from doing so)
  • Hopefully all of them!

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 14, 2022

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

@SiobhyB SiobhyB mentioned this pull request Apr 14, 2022
3 tasks
@SiobhyB SiobhyB added the i18n label Apr 14, 2022
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 14, 2022

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

@SiobhyB
Copy link
Contributor Author

SiobhyB commented Apr 19, 2022

@ravishanker, @develric, 👋 , I've requested you as reviewers as you'd originally reviewed the initial PR (and it seemed like it was close to being approved). Let me know if anything needs changing here or if there are perhaps other reviewers who should be added. I'd love to try to get this over the finish line. 🙇‍♀️

For further context, the reason I'm trying to get the improved locale picker merged is that I'd like to fix the following issue with missing language options in the site settings: #16312. That fix will involve the addition of ~100 more language options under site settings, which wouldn't work very well with the existing locale picker.

@SiobhyB SiobhyB marked this pull request as ready for review April 19, 2022 13:04
Copy link
Member

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Works as expected! Thanks for updating it to work with trunk. I think we addressed all the issues in the original PR, and just never got to actually merging it.

@SiobhyB SiobhyB added this to the 19.8 milestone Apr 19, 2022
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Apr 27, 2022

Thank you, @khaykov! 🙇‍♀️ @ravishanker, @develric, if you have no objections, I'll go ahead to merge this PR tomorrow.

Copy link
Contributor

@ravishanker ravishanker left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this. 🙇
LGTM 👍

}
})

viewModel.hideKeyboard.observe(viewLifecycleOwner, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lambda argument can be moved outside parenthesis like so. Update all of them

viewModel.hideKeyboard.observe(viewLifecycleOwner) {
        ActivityUtils.hideKeyboardForced(binding?.searchInputLayout)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 158d11e, thank you for reviewing! 🙌

@SiobhyB SiobhyB enabled auto-merge April 28, 2022 15:54
@SiobhyB SiobhyB disabled auto-merge April 28, 2022 17:32
Siobhan added 2 commits April 28, 2022 18:37
This notation was mistakenly removed following previous updates from trunks.
@SiobhyB SiobhyB enabled auto-merge April 28, 2022 17:49
@SiobhyB SiobhyB merged commit 0532250 into trunk Apr 28, 2022
@SiobhyB SiobhyB deleted the feature/updated-locale-picker branch April 28, 2022 18:02
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