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

For #21437 - Relocated Home-related settings to its dedicated sub screen #21722

Merged
merged 9 commits into from
Oct 6, 2021

Conversation

MozillaNoah
Copy link
Contributor

@MozillaNoah MozillaNoah commented Oct 5, 2021

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

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

Screenshot_1633468703

@MozillaNoah MozillaNoah requested review from a team as code owners October 5, 2021 21:23
@MozillaNoah MozillaNoah added the needs:review PRs that need to be reviewed label Oct 5, 2021
@MozillaNoah
Copy link
Contributor Author

I'll hold off on merging until I get a green light from @betsymi and @topotropic.

Comment on lines 384 to 402
var startOnHomeAfterFourHours by booleanPreference(
appContext.getPreferenceKey(R.string.pref_key_start_on_home_after_four_hours),
appContext.getPreferenceKey(R.string.pref_key_opening_screen_homepage),
default = true
)

/**
* Indicates if the user has selected the option to always start on the home screen.
*/
var startOnHomeAlways by booleanPreference(
appContext.getPreferenceKey(R.string.pref_key_start_on_home_always),
appContext.getPreferenceKey(R.string.pref_key_opening_screen_last_tab),
default = false
)

/**
* Indicates if the user has selected the option to never start on the home screen.
*/
var startOnHomeNever by booleanPreference(
appContext.getPreferenceKey(R.string.pref_key_start_on_home_never),
appContext.getPreferenceKey(R.string.pref_key_opening_screen_after_four_hours_of_inactivity),
default = false
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are changing they keys, should we also change the variable names and the docs to reflect the new behavior? :)

Copy link
Contributor Author

@MozillaNoah MozillaNoah Oct 6, 2021

Choose a reason for hiding this comment

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

I admittedly changed them in the fragment but not here. I'll update them here too. (The behavior is the same though)

Copy link
Contributor

@Amejia481 Amejia481 Oct 6, 2021

Choose a reason for hiding this comment

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

Thanks for the clarification, if the behavior is the same, why we are changing the keys? This is going to cause that we loose the previous user choice? Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't even consider that implication. Thank you for catching that. I'll revert this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No problem :)

Comment on lines -73 to -83
private fun assertStartOnHomeOptions() {
// Scroll to ensure all the items are visible.
scrollToElementByText("Never")
startOnHomeHeading()
.check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
afterFourHoursToggle()
.check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
alwaysStartOnHomeToggle()
.check(matches(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE)))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

After merging, let's notify the QA team on #mobile-test-alerts, that we are removing this test and they will need to update it to reflect the new UI :)

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

@MozillaNoah MozillaNoah added pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] and removed needs:review PRs that need to be reviewed labels Oct 6, 2021
@mergify mergify bot merged commit 1f97ca6 into mozilla-mobile:main Oct 6, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Mar 7, 2022
…icated sub screen (mozilla-mobile#21722)

* For mozilla-mobile#21437 - Relocated Home-related settings to its dedicated sub screen

* For mozilla-mobile#21437 - Updated show top sites toggle text

* PR: Fixed lint warning. Reverted preference keys

* PR: added ignore for UI test

* PR: Added ignore for UI test
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants