For FNX-22339: UI components for recently added bookmarks #19953

eliserichards · 2021-06-10T18:07:38Z

UI components for recently added bookmarks on the homescreen. This is broken out of #19835 so it's not so large. Diagram for how these are broken down: https://docs.google.com/drawings/d/1oiHqRl9f_LwsLbQ98zUBjChyDn5xm7us-iDF4rqhRak/edit?usp=sharing

Header with "show all" button
Bookmark item
"Component" to house the items (contains recent_bookmarks_list)
New styles & dimens to match the designs

Screenshots

header with show all button

button padding (accessibility)

Bookmark item with image, title, and subtitle

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:

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

mcarare · 2021-06-10T19:06:07Z

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

+    android:layout_height="@dimen/recent_bookmark_item_height"
+    android:orientation="vertical">
+
+    <com.google.android.material.card.MaterialCardView


Just dropping a passing-by comment: favicon_card is the single child of the ConstraintLayout. Is this intentional?

Yes! You can see the difference between this file and top_site_item: for the top site cards, we want a card view with a title beneath it. For these bookmarks, we want the text and and the icon/image inside the card itself (lines 29/44)

(Link to the design if you're curious: https://www.figma.com/file/am9EJGg6ow29PXWkc9jcOH/MR2.1?node-id=11%3A396)

mergify · 2021-06-10T20:23:08Z

This pull request has conflicts when rebasing. Could you fix it @eliserichards? 🙏

codecov-commenter · 2021-06-10T23:29:38Z

Codecov Report

Merging #19953 (79888d8) into master (9d3cf79) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 79888d8 differs from pull request most recent head f3d13a7. Consider uploading reports for the commit f3d13a7 to get more accurate results

@@             Coverage Diff              @@
##             master   #19953      +/-   ##
============================================
- Coverage     36.87%   36.86%   -0.01%     
- Complexity     1629     1630       +1     
============================================
  Files           541      541              
  Lines         21021    21021              
  Branches       3124     3124              
============================================
- Hits           7751     7750       -1     
+ Misses        12379    12378       -1     
- Partials        891      893       +2

Impacted Files	Coverage Δ
...rg/mozilla/fenix/settings/account/AccountUiView.kt	`34.14% <0.00%> (-2.44%)`	⬇️
...in/java/org/mozilla/fenix/perf/NavGraphProvider.kt	`62.50% <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 9d3cf79...f3d13a7. Read the comment docs.

jonalmeida · 2021-06-10T23:29:55Z

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

+    android:layout_height="match_parent"
+    android:isScrollContainer="true"
+    android:gravity="start">
+


Shouldn't the contents of recent_bookmarks_header be here (above the RecyclerView)? Is there a situation where you can have a bookmarks header without the list?

You can use <include layout="" /> if you want to keep some separation of concerns between the header and this instead of in-lining.

Great point! I want to keep them separate for now in case we decide to do a placeholder component (like collections), so I think <include layout..> would be a good solution.

gabrielluong

I only looked at the header. You probably want another review for the bookmarks component.

gabrielluong · 2021-06-11T15:19:53Z

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

@@ -0,0 +1,39 @@
+<?xml version="1.0" encoding="utf-8"?>


Instead of having 2 exactly identical layouts, we might want to refactor https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/res/layout/recent_tabs_header.xml into recent_header.xml and specify the header.text and header.contentDescription in the view holder instead.

We managed to align the blueprint to be similar to the figma frame structure (think of this as header bounding box that you can see in the layers in figma), which you might appreciate.

This can be in a different PR. There are a number of things that my components are going to add that are missing from first glance in the "Jump back in" pr and I'd rather get this landed first then deal with that.

In that case, I think you can duplicate https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/res/layout/recent_tabs_header.xml more or less here. It will help you cut back on the number of variables and really simplify your layout.

I'd prefer to keep it as-is

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

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

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

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

gabrielluong · 2021-06-11T15:51:32Z

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

@@ -403,7 +403,14 @@
        <item name="android:textColor">?primaryText</item>
    </style>

+    <style name="Body12TextStyle" parent="TextAppearance.MaterialComponents.Body1">
+        <item name="android:textColor">@color/photonDarkGrey05</item>


Same as below, but we should probably stick to using:

item name="android:textColor">?primaryText</item>

to keep these generic Body text styles consistent.

It's not the primaryText color.. I'll have to see if one exists for photonDarkGrey05 and if not I'll set one up

looks like "Jump back in" tabs is using a pre-defined color as well (<color name="home_show_all_button_text">@color/photonDarkGrey05</color>). I'm going to keep this as-is for now and fix them together in a separate PR when we get the Dark values

I got the dark theme colors from Nicole. Fixed it here and filed a new issue to add them to "Jump back in"

I got the dark theme colors from Nicole. Fixed it here and filed a new issue to add them to "Jump back in"

Ah yes, we just added home_show_all_button_text. It is using photoLightGrey50 for the dark theme in the "Show all" button - see https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/res/values-night/colors.xml#99 (note the values-nightly/colors.xml). I checked with Nicole about the header colours since I didn't see this response before. I still hold that we probably want to keep the textColor as ?primaryText in these BodyXXTextStyle since it would be strange to have "12" be different from the rest and we can just add the textColor locally.

The "Jump back in" header actually also uses the correct dark theme color with using style="@style/Header20TextStyle". In our existing color variables and component system, ?primaryText maps to https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/res/values-night/colors.xml#7 which maps to https://searchfox.org/mozilla-mobile/source/fenix/app/src/main/res/values/colors.xml#80 which is photonLightGrey05. I will close the Jira issue since there's no change necessary.

You might be interested in reusing home_show_all_button_text since we anticipated the need for a generic "Show all" button text color.

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

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

gabrielluong · 2021-06-11T17:13:20Z

app/src/main/res/color/primary_state_button_text_color.xml

+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+<selector xmlns:android="http://schemas.android.com/apk/res/android">


I feel this change was in response to the dark theme feedback. I am not entirely sure since we can't really test its usage since it's not being used. However, if this is to adjust for dark theme, we actually do this quite differently without selectors and in nightly/colors.xml

This is being used in another PR. I just want to get this landed.

To be clear, there is additional work needed to add dark themes for "Jump back in" as well. This doesn't have to happen in this PR.

I guess this is what makes the PR and review a bit weird since it's just xml.

I believe I made it clear why this was broken out. You can see the link to the main PR in the description

I am wondering what the additional work needed for the dark theme header since we already use ?primaryText in Header20TextStyle. I think that's probably even more reasons we should just not have 2 headers and aim to write a single component.

Yes, and this will happen in another PR.

gabrielluong · 2021-06-11T18:21:12Z

app/src/main/res/color/recent_bookmark_item_text_color.xml

+<!-- This Source Code Form is subject to the terms of the Mozilla Public
+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+<selector xmlns:android="http://schemas.android.com/apk/res/android">


There's nothing wrong with using <selector> for the different text color state between light and dark theme. I personally would be interested in getting feedback from everyone on whether or not we should be moving towards doing it this way or just adding the color variables in colors.xml and values-night/colors.xml (https://searchfox.org/mozilla-mobile/search?q=home_show_all_button_text&path= for example).

jonalmeida · 2021-06-11T19:13:02Z

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

+        <item name="cardElevation">@dimen/recent_bookmark_item_card_elevation</item>
+    </style>
+
+    <style name="recentBookmarkFavicon">


In general, we should avoid adding these kinds of styles in future work, it's a strange precedence that exists in the code base because, the way we're using it in Fenix, doesn't seem to add any value from just setting these one-time used styles directly in the layout files.

Nothing to change here for now, just food for thought.

That's good to know. We definitely don't want to make styles so huge that it becomes unusable

gabrielluong · 2021-06-11T19:21:25Z

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

@@ -403,7 +403,14 @@
        <item name="android:textColor">?primaryText</item>
    </style>

+    <style name="Body12TextStyle" parent="TextAppearance.MaterialComponents.Body1">
+        <item name="android:textColor">@color/primary_state_title_text_color</item>


Sorry, I am gonna blocked this on one change here, which might've been missed in the comments #19953 (comment). Realistically, I think this textColor should be define in recentBookmarkItemSubTitleText as Body12 shouldn't be using a different text color compare to all its BodyXXTextStyle.

Suggested change

<item name="android:textColor">@color/primary_state_title_text_color</item>

<item name="android:textColor">?primaryText</item>

This is happening in the other PR, and I can't do that until this is merged.

Also, this is not supposed to be ?primaryText color, it's a different value

Also, this is not supposed to be ?primaryText color, it's a different value

I think there's some misunderstanding here. We want this to be ?primaryText here since all the other BodyXXTextStyle uses ?primaryText.

Since Body12TextStyle is extended in recentBookmarkItemSubTitleText, we can just move <item name="android:textColor">@color/primary_state_title_text_color</item> over.

N/A

…bile#19953) * Top level layout with recyclerview for recent bookmarks * Add layout for recent bookmarked item, including strings and styles. * Header layout for recently saved bookmarks * Address review comments * Include the header for recent bookmarks in the component layout * Add dark theme colors for button and title styles * Recent bookmark card title text color for dark mode/light mode

eliserichards requested review from a team as code owners June 10, 2021 18:07

eliserichards force-pushed the fnx-22339-ui-components branch from 8e898c3 to a2855ad Compare June 10, 2021 18:08

eliserichards added the needs:review PRs that need to be reviewed label Jun 10, 2021

eliserichards mentioned this pull request Jun 10, 2021

For FNX-22339: Recently saved bookmarks #19835

Merged

10 tasks

mcarare reviewed Jun 10, 2021

View reviewed changes

eliserichards closed this Jun 10, 2021

eliserichards reopened this Jun 10, 2021

eliserichards added 3 commits June 10, 2021 18:09

Top level layout with recyclerview for recent bookmarks

8bd320a

Add layout for recent bookmarked item, including strings and styles.

ea68be3

Header layout for recently saved bookmarks

a8c1902

eliserichards force-pushed the fnx-22339-ui-components branch from a2855ad to a8c1902 Compare June 10, 2021 23:11

jonalmeida reviewed Jun 10, 2021

View reviewed changes

gabrielluong reviewed Jun 11, 2021

View reviewed changes

eliserichards added 2 commits June 11, 2021 11:54

Address review comments

9635e5b

Include the header for recent bookmarks in the component layout

cbca21c

eliserichards requested a review from jonalmeida June 11, 2021 16:56

Add dark theme colors for button and title styles

79888d8

gabrielluong reviewed Jun 11, 2021

View reviewed changes

Recent bookmark card title text color for dark mode/light mode

f3d13a7

gabrielluong reviewed Jun 11, 2021

View reviewed changes

jonalmeida approved these changes Jun 11, 2021

View reviewed changes

gabrielluong previously requested changes Jun 11, 2021

View reviewed changes

eliserichards merged commit e373f50 into mozilla-mobile:master Jun 11, 2021

eliserichards removed the needs:review PRs that need to be reviewed label Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For FNX-22339: UI components for recently added bookmarks #19953

For FNX-22339: UI components for recently added bookmarks #19953

eliserichards commented Jun 10, 2021 •

edited

mcarare Jun 10, 2021

eliserichards Jun 10, 2021 •

edited

mergify bot commented Jun 10, 2021

codecov-commenter commented Jun 10, 2021 •

edited

jonalmeida Jun 10, 2021

eliserichards Jun 11, 2021

gabrielluong left a comment

gabrielluong Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021

eliserichards Jun 11, 2021

eliserichards Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021

gabrielluong Jun 11, 2021 •

edited

eliserichards Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021 •

edited

eliserichards Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021

jonalmeida Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021 •

edited

eliserichards Jun 11, 2021

eliserichards Jun 11, 2021

gabrielluong Jun 11, 2021 •

edited

	<item name="android:textColor">@color/primary_state_title_text_color</item>
	<item name="android:textColor">?primaryText</item>

For FNX-22339: UI components for recently added bookmarks #19953

For FNX-22339: UI components for recently added bookmarks #19953

Conversation

eliserichards commented Jun 10, 2021 • edited

Screenshots

Pull Request checklist

To download an APK when reviewing a PR:

Choose a reason for hiding this comment

eliserichards Jun 10, 2021 • edited

Choose a reason for hiding this comment

mergify bot commented Jun 10, 2021

codecov-commenter commented Jun 10, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Jun 11, 2021 • edited

Choose a reason for hiding this comment

eliserichards Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielluong Jun 11, 2021 • edited

Choose a reason for hiding this comment

eliserichards commented Jun 10, 2021 •

edited

eliserichards Jun 10, 2021 •

edited

codecov-commenter commented Jun 10, 2021 •

edited

gabrielluong Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021 •

edited

eliserichards Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021 •

edited

gabrielluong Jun 11, 2021 •

edited