-
Notifications
You must be signed in to change notification settings - Fork 328
Bug 1876752 - Reordered preferences in SettingsFragment
to match the UI layout
#5321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -42,10 +41,8 @@ | |||
<string name="pref_key_override_amo_user" translatable="false">pref_key_override_amo_user</string> | |||
<string name="pref_key_override_amo_collection" translatable="false">pref_key_override_amo_collection</string> | |||
<string name="pref_key_enable_gecko_logs" translatable="false">pref_key_enable_gecko_logs</string> | |||
<string name="pref_key_help" translatable="false">pref_key_help</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't you cleaning these up in #5320?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MozillaNoah yep! This PR depends on that issue, which is why it's included as a commit here. I wanted to keep the 2 refactors separate 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really need to have coffee before doing your PRs 😂
// Only displayed when secret settings are enabled | ||
resources.getString(R.string.pref_key_install_local_addon) -> { | ||
addonFilePicker.launch() | ||
null | ||
} | ||
// Only displayed when secret settings are enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add a new line in-between for these since we did it for everything else? This also needs to happen below so you may need to go through your changes again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot, the formatter must've ignored it because of the comments
This pull request has conflicts when rebasing. Could you fix it @t-p-white? 🙏 |
Remove redundant preferences from SettingsFragment being merged first.
No new preferences have been added, this is solely re-arranging the existing preferences to match the UI ordering where applicable.
Pull Request checklist
After merge
To download an APK when reviewing a PR (after all CI tasks finished running):
Checks
at the top of the PR page.firefoxci-taskcluster
group on the left to expand all tasks.build-apk-{fenix,focus,klar}-debug
task you're interested in.View task in Taskcluster
in the newDETAILS
section.GitHub Automation
https://bugzilla.mozilla.org/show_bug.cgi?id=1876752