Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

for #24929: remove locale restriction for remote firefox wallpapers #24930

Merged
merged 2 commits into from
Apr 22, 2022
Merged

for #24929: remove locale restriction for remote firefox wallpapers #24930

merged 2 commits into from
Apr 22, 2022

Conversation

MatthewTighe
Copy link
Contributor

for #24929

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

@MatthewTighe MatthewTighe requested review from a team as code owners April 22, 2022 19:04
@MatthewTighe MatthewTighe reopened this Apr 22, 2022
Amejia481
Amejia481 approved these changes Apr 22, 2022
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

LGTM!

app/src/main/java/org/mozilla/fenix/home/HomeFragment.kt Outdated Show resolved Hide resolved
@MatthewTighe MatthewTighe force-pushed the firefox-wallpapers-for-all-locales branch 2 times, most recently from 7549e42 to 665e5e4 Compare April 22, 2022 19:39
@Amejia481 Amejia481 self-requested a review April 22, 2022 19:41
Amejia481
Amejia481 reviewed Apr 22, 2022
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

Looks like we have an issue, in the wallpapers page when we enter there with a local that is not promotional, it looks like we are trying to display all wallpapers available.

image

@MatthewTighe
Copy link
Contributor Author

This should be fixed in my most recent push, I'd also just noticed it. Thanks for the thorough check!

@MatthewTighe MatthewTighe force-pushed the firefox-wallpapers-for-all-locales branch from 665e5e4 to 43f5720 Compare April 22, 2022 20:56
@MatthewTighe MatthewTighe force-pushed the firefox-wallpapers-for-all-locales branch from 43f5720 to 6dde8e2 Compare April 22, 2022 21:07
gabrielluong
gabrielluong reviewed Apr 22, 2022
View reviewed changes
app/src/main/java/org/mozilla/fenix/wallpapers/Wallpaper.kt
override val remoteParentDirName: String = "house"
override fun isAvailableInLocale(locale: String): Boolean =
listOf("en-US", "es-US").contains(locale)
Copy link
Member

Choose a reason for hiding this comment

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

In case this just happens to be useful information, we can tell a user's region based on their store.state.search.region. If we were trying to make the wallpaper available for all of US instead of just based on their locale. See https://github.com/mozilla-mobile/fenix/pull/17580/files#diff-3101d2ecb99584cfe0d93898bc7eee5a629ddf6de9f2d2632f56fb3f04e2c34dR395 for an example.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's nice!!!
We could add it as a follow up ticket :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is actually really good to know. I think when we talked about this initially, we didn't think there was another good way for us to determine locale. That's why we included es-US, since it's another high usage locale in the US.

@MatthewTighe MatthewTighe force-pushed the firefox-wallpapers-for-all-locales branch from 6dde8e2 to 6fe6373 Compare April 22, 2022 21:20
@MatthewTighe MatthewTighe force-pushed the firefox-wallpapers-for-all-locales branch from 6fe6373 to ecdb3ee Compare April 22, 2022 21:22
@Amejia481 Amejia481 requested review from gabrielluong and removed request for gabrielluong April 22, 2022 21:38
@Amejia481
Copy link
Contributor

🚢 !

@Amejia481 Amejia481 requested review from gabrielluong and Amejia481 and removed request for gabrielluong April 22, 2022 21:40
@MatthewTighe MatthewTighe added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Apr 22, 2022
@mergify mergify bot merged commit 0f95228 into mozilla-mobile:main Apr 22, 2022
@MatthewTighe
Copy link
Contributor Author

@Mergifyio backport releases_v100.0.0

@mergify
Copy link
Contributor

mergify bot commented Apr 22, 2022

backport releases_v100.0.0

✅ Backports have been created

@mergify mergify bot mentioned this pull request Apr 22, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gabrielluong gabrielluong gabrielluong left review comments

@Amejia481 Amejia481 Amejia481 approved these changes

Assignees
No one assigned
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MatthewTighe @Amejia481 @gabrielluong