Bug 1821430 - Update strings in the ETP settings screen part 2. Use the new strings and update the layout #1213

t-p-white · 2023-03-13T17:05:06Z

Still todo: Update SUMO with screenshots when approved.

ℹ️ Please see Figma model for exact layout specifications.

App (updated)	Design

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1821430

fenix/app/src/main/res/drawable/ic_etp_artwork.xml

fenix/app/src/main/res/layout/tracking_protection_learn_more_preference.xml

fenix/app/src/main/res/xml/tracking_protection_preferences.xml

mavduevskiy

Moving into the right direction! I left some notes on styling and ui elements spacing!

fenix/app/src/main/res/drawable/ic_etp_artwork.xml

fenix/app/src/androidTest/java/org/mozilla/fenix/ui/EnhancedTrackingProtectionTest.kt

fenix/app/src/debug/res/layout/preference_no_widget.xml

fenix/app/src/debug/res/layout/preference_widget_switch_fenix_style.xml

fenix/app/src/main/java/org/mozilla/fenix/settings/FenixSwitchPreference.kt

mavduevskiy · 2023-03-28T08:16:20Z

fenix/app/src/main/res/layout/preference_widget_radiobutton_with_info.xml

@@ -10,8 +10,7 @@
    android:layout_height="wrap_content"
    android:baselineAligned="false"
    android:orientation="horizontal"
-    android:paddingTop="@dimen/radio_button_preference_vertical"
-    android:paddingBottom="@dimen/radio_button_preference_vertical">
+    android:paddingBottom="12dp">


This file, given the design, needs some adjustments.

Top spacing from the title should be 6dp, bottom spacing from the widget_summary should be 6dp as well. The info icon should be positioned in center (currently bottom padding of the parent view is lifting it up a bit).

Here is the comparison between xml file and figma design. Spacing is a bit off! Top spacing of the title and spacing between the checkbox and the text are off.

Good spot, a rogue top margin on title. Updated per screenshot

mavduevskiy · 2023-03-28T08:32:25Z

fenix/app/src/main/res/layout/tracking_protection_learn_more_preference.xml

-        android:paddingTop="12dp"
-        android:paddingBottom="12dp"
+        android:layout_marginStart="16dp"
+        android:layout_marginTop="16dp"


this file still looks very different compared to the new design! Top/bottom spacing are different, layout_constraintGuide_percent value is too high. Even the image itself looks a bit different (in the design the top element in the image is the cloud, but in the file we actually have the sun as the top element, hence top padding is different)

Yes, the provided image is actually slightly different to the Figma design. I've cc'd you into some UX conversations around this ETP banner, which should provide some context 🙂

mavduevskiy · 2023-03-28T21:44:21Z

fenix/app/src/main/res/layout/preference_widget_switch_fenix_style.xml

+        android:letterSpacing="0.01"
+        android:lineSpacingExtra="5sp"
+        android:textAppearance="@style/Header16TextStyle"
+        app:layout_constraintStart_toStartOf="parent"


let's add constraints to the text view similar to the summary. if the string is too long, it will violate the checkbox space :)

mavduevskiy · 2023-03-28T21:46:59Z

fenix/app/src/main/res/layout/preference_widget_switch_fenix_style.xml

+        android:layout_marginTop="12dp"
+        android:gravity="center_vertical"
+        android:letterSpacing="0.01"
+        android:lineSpacingExtra="5sp"


let's add bottom padding to the textView, to follow the design. lineSpacingExtra plays a role only in between lines, but doesn't affect the spacing below the last line. If you check the design, the line height on the textView is 24dp, which gives us 4dp extra padding to the bottom of 16sp text :)

…he new strings and update the layout

mavduevskiy

looks good! 🚢 it!

t-p-white added the 🕵️‍♀️ needs review PRs that need to be reviewed label Mar 13, 2023

t-p-white requested a review from rocketsroger March 13, 2023 17:05

t-p-white commented Mar 13, 2023

View reviewed changes

fenix/app/src/main/res/drawable/ic_etp_artwork.xml Show resolved Hide resolved

t-p-white force-pushed the 1821430_2 branch from 24e7e9d to 155e377 Compare March 13, 2023 18:24

mavduevskiy self-requested a review March 14, 2023 05:00

This comment was marked as outdated.

Sign in to view

mavduevskiy self-requested a review March 14, 2023 05:08

mavduevskiy reviewed Mar 14, 2023

View reviewed changes

fenix/app/src/main/res/layout/tracking_protection_learn_more_preference.xml Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

t-p-white force-pushed the 1821430_2 branch from befbc53 to da3888f Compare March 14, 2023 12:14

t-p-white commented Mar 14, 2023

View reviewed changes

fenix/app/src/main/res/xml/tracking_protection_preferences.xml Outdated Show resolved Hide resolved

t-p-white requested a review from mavduevskiy March 14, 2023 12:22

t-p-white added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Mar 15, 2023

t-p-white force-pushed the 1821430_2 branch 5 times, most recently from 267e906 to da2d1d8 Compare March 16, 2023 12:40

t-p-white added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Mar 20, 2023

This comment was marked as resolved.

Sign in to view

t-p-white force-pushed the 1821430_2 branch 4 times, most recently from f44d760 to bfb5cad Compare March 24, 2023 17:21

mavduevskiy reviewed Mar 28, 2023

View reviewed changes

t-p-white added the Needs-UX Issues or PRs that need UX input or review label Mar 28, 2023

mavduevskiy reviewed Mar 28, 2023

View reviewed changes

t-p-white removed the Needs-UX Issues or PRs that need UX input or review label Mar 29, 2023

t-p-white force-pushed the 1821430_2 branch 2 times, most recently from a120996 to 4ab464d Compare March 29, 2023 17:27

Bug 1821430 - Update strings in the ETP settings screen part 2. Use t…

4dcbc33

…he new strings and update the layout

t-p-white force-pushed the 1821430_2 branch from 4ab464d to 4dcbc33 Compare March 29, 2023 17:31

mavduevskiy self-requested a review March 29, 2023 17:31

mavduevskiy approved these changes Mar 29, 2023

View reviewed changes

t-p-white added approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Mar 30, 2023

Merge branch 'main' into 1821430_2

6eeb598

t-p-white added the 🛬 needs landing (squash) PRs that are ready to land (squashed) label Mar 31, 2023

Merge branch 'main' into 1821430_2

a8f0f93

mergify bot merged commit 4ea5308 into mozilla-mobile:main Mar 31, 2023
21 checks passed

t-p-white deleted the 1821430_2 branch April 27, 2023 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1821430 - Update strings in the ETP settings screen part 2. Use the new strings and update the layout #1213

Bug 1821430 - Update strings in the ETP settings screen part 2. Use the new strings and update the layout #1213

t-p-white commented Mar 13, 2023 •

edited by github-actions bot

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

mavduevskiy left a comment

mavduevskiy Mar 28, 2023

t-p-white Mar 28, 2023

mavduevskiy Mar 28, 2023

t-p-white Mar 29, 2023

mavduevskiy Mar 28, 2023 •

edited

t-p-white Mar 28, 2023

mavduevskiy Mar 28, 2023

mavduevskiy Mar 28, 2023

mavduevskiy left a comment

Bug 1821430 - Update strings in the ETP settings screen part 2. Use the new strings and update the layout #1213

Bug 1821430 - Update strings in the ETP settings screen part 2. Use the new strings and update the layout #1213

Conversation

t-p-white commented Mar 13, 2023 • edited by github-actions bot

Pull Request checklist

After merge

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

mavduevskiy left a comment

Choose a reason for hiding this comment

mavduevskiy Mar 28, 2023

Choose a reason for hiding this comment

t-p-white Mar 28, 2023

Choose a reason for hiding this comment

mavduevskiy Mar 28, 2023

Choose a reason for hiding this comment

t-p-white Mar 29, 2023

Choose a reason for hiding this comment

mavduevskiy Mar 28, 2023 • edited

Choose a reason for hiding this comment

t-p-white Mar 28, 2023

Choose a reason for hiding this comment

mavduevskiy Mar 28, 2023

Choose a reason for hiding this comment

mavduevskiy Mar 28, 2023

Choose a reason for hiding this comment

mavduevskiy left a comment

Choose a reason for hiding this comment

t-p-white commented Mar 13, 2023 •

edited by github-actions bot

mavduevskiy Mar 28, 2023 •

edited