For #9857: Create button style #9858

sblatz · 2020-04-10T17:49:04Z

Note: this is the first step for implementing this. Ideally this will live in AC, but I want to get something merged in for now so we don't make the problem worse!

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.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

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

kglazko

Code looks great! One question and one thing to note, which I commented. I still need to compare some things to Master visually before I can approve.

kglazko · 2020-04-10T18:25:21Z

app/src/main/res/layout/save_tab_group_button.xml

-    app:icon="@drawable/ic_tab_collection"
-    app:iconGravity="textStart"
-    app:iconPadding="8dp"
-    app:fontFamily="@font/metropolis_medium"


You may want to verify with UX cases like this here in that this button was one of the few and only cases in which semi-bold was too bold and they requested medium instead. We have iterated on this a couple of times though, but may want to check with them to see whether they're happy with it in semi-bold or medium. In your implementation, you inherit the styling for the positive button from the neutral one, there may be a couple more cases like this as you go through headers etc. later where it may require a different font than from the default button.

I will check with them before I get UX sign off! 😄

kglazko · 2020-04-10T18:31:15Z

app/src/main/res/values/styles.xml

-    <style name="ThemeIndependentMaterialGreyButton" parent="Widget.MaterialComponents.Button.TextButton">
+    <!-- UI button styling -->
+    <style name="NeutralButton" parent="Widget.MaterialComponents.Button.TextButton">
+        <item name="iconTint">@color/button_text_color</item>


Are you adding new styling for icons in buttons here?

My understanding from UX is that the icons should match the text tint :) another thing I will confirm in the style guide before it's formalized.

kglazko · 2020-04-10T18:31:47Z

app/src/main/res/values/styles.xml

        <item name="android:textStyle">bold</item>
        <item name="android:textAllCaps">false</item>
        <item name="backgroundTint">@color/grey_button_color</item>
        <item name="android:textColor">@color/button_text_color</item>
        <item name="rippleColor">?secondaryText</item>
+        <item name="android:letterSpacing">0</item>


Why did the letter spacing need to be specified here and/or is different from the other styles?

The letters looked super spread out when I didn't change this. It's defined as "0" in several places throughout the app, and I think it should be a consistent thing. I will confirm with UX and file a follow up for addressing this, but I think 0 is fine for now?

kglazko · 2020-04-10T18:44:50Z

mozilla-lint-rules/src/main/java/org/mozilla/fenix/lintrules/ButtonStyleXmlDetector.kt

+import com.android.tools.lint.detector.api.Severity
+import com.android.tools.lint.detector.api.XmlContext
+import org.w3c.dom.Element
+


A KDOC here would be nice

codecov-io · 2020-04-10T18:47:39Z

Codecov Report

Merging #9858 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #9858   +/-   ##
=========================================
  Coverage     19.16%   19.17%           
  Complexity      521      521           
=========================================
  Files           337      337           
  Lines         13738    13738           
  Branches       1839     1839           
=========================================
+ Hits           2633     2634    +1     
  Misses        10865    10865           
+ Partials        240      239    -1

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/org/mozilla/fenix/components/Search.kt	`100.00% <0.00%> (+8.33%)`	`2.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c23840...e97a019. Read the comment docs.

ekager · 2020-04-15T02:56:15Z

Looks like there are some parts of the app that use a TextView embedded in a FrameLayout with background to get the look of a MaterialButton. Not really sure why... But could we try fix these to adopt the same styles as well?
For example, this is in onboarding_private_browsing and onboarding_privacy_notice. Those are the only two I saw with a quick search but there could be more :/

sblatz requested a review from kglazko April 10, 2020 17:49

sblatz force-pushed the button-as-style branch from cc72ad2 to a2dded0 Compare April 10, 2020 17:52

kglazko reviewed Apr 10, 2020

View reviewed changes

sblatz force-pushed the button-as-style branch from 8eb39e0 to e97a019 Compare April 13, 2020 17:45

sblatz changed the title ~~For #9857: Create button style~~ For #9857: Create button style RUN Apr 13, 2020

sblatz changed the title ~~For #9857: Create button style RUN~~ For #9857: Create button style Apr 13, 2020

kglazko approved these changes Apr 15, 2020

View reviewed changes

sblatz added 3 commits April 15, 2020 13:10

For mozilla-mobile#9857: Creates text button style

168ba95

Adds lint check

80c88f7

Fixes onboarding cards

30db324

sblatz force-pushed the button-as-style branch from e97a019 to 30db324 Compare April 15, 2020 21:03

sblatz merged commit 8185ba7 into mozilla-mobile:master Apr 15, 2020

This was referenced Apr 16, 2020

For #8422: Add misc migration UI fixes #9945

Merged

Work on componetizing buttons #9298

Closed

liuche mentioned this pull request Apr 28, 2020

Releng 5.0 #10205

Closed

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #9857: Create button style #9858

For #9857: Create button style #9858

sblatz commented Apr 10, 2020 •

edited

kglazko left a comment

kglazko Apr 10, 2020

sblatz Apr 10, 2020

kglazko Apr 10, 2020

sblatz Apr 10, 2020

kglazko Apr 10, 2020

sblatz Apr 10, 2020 •

edited

kglazko Apr 10, 2020

sblatz Apr 13, 2020

codecov-io commented Apr 10, 2020 •

edited

ekager commented Apr 15, 2020

For #9857: Create button style #9858

For #9857: Create button style #9858

Conversation

sblatz commented Apr 10, 2020 • edited

Pull Request checklist

After merge

To download an APK when reviewing a PR:

kglazko left a comment

Choose a reason for hiding this comment

kglazko Apr 10, 2020

Choose a reason for hiding this comment

sblatz Apr 10, 2020

Choose a reason for hiding this comment

kglazko Apr 10, 2020

Choose a reason for hiding this comment

sblatz Apr 10, 2020

Choose a reason for hiding this comment

kglazko Apr 10, 2020

Choose a reason for hiding this comment

sblatz Apr 10, 2020 • edited

Choose a reason for hiding this comment

kglazko Apr 10, 2020

Choose a reason for hiding this comment

sblatz Apr 13, 2020

Choose a reason for hiding this comment

codecov-io commented Apr 10, 2020 • edited

Codecov Report

ekager commented Apr 15, 2020

sblatz commented Apr 10, 2020 •

edited

sblatz Apr 10, 2020 •

edited

codecov-io commented Apr 10, 2020 •

edited