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

For #21339: update styles for homescreen to match new specs #21342

Merged

Conversation

eliserichards
Copy link
Contributor

For #21339

Updates the "show all" buttons on the home screen. Adds correct spacing in between jump back in cards.

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

@eliserichards eliserichards added the needs:review PRs that need to be reviewed label Sep 16, 2021
@eliserichards eliserichards requested review from a team as code owners September 16, 2021 20:52
@eliserichards eliserichards linked an issue Sep 16, 2021 that may be closed by this pull request
@eliserichards
Copy link
Contributor Author

eliserichards commented Sep 16, 2021

Failing UI tests:

  • verifyExpandedCollectionItemsTest: works locally ✅
  • createFirstCollectionTest: works locally ✅
  • verifyAddTabButtonOfCollectionMenu: works locally ✅
  • renameCollectionTest: works locally ✅

@eliserichards eliserichards added pr:do-not-land pr:waiting-for-authors PR that has been approved and awaiting any changes before they can land and removed needs:review PRs that need to be reviewed labels Sep 20, 2021
@eliserichards
Copy link
Contributor Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2021

Command rebase: success

Branch has been successfully rebased

@mergify
Copy link
Contributor

mergify bot commented Sep 20, 2021

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

@eliserichards
Copy link
Contributor Author

eliserichards commented Sep 20, 2021

Same UI tests failing again, and all run fine locally:

  • verifyExpandedCollectionItemsTest: logs
  • createFirstCollectionTest: logs
  • verifyAddTabButtonOfCollectionMenu: logs
  • renameCollectionTest: logs

Adding ignores and filing a bug for help: #21397

Copy link
Member

@gabrielluong gabrielluong left a comment

Choose a reason for hiding this comment

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

Will pull down and test it out after lunch

app/src/main/res/layout/history_metadata_header.xml Outdated Show resolved Hide resolved
@@ -93,7 +93,7 @@
<color name="collection_icon_color_yellow">@color/collection_icon_color_yellow_dark_theme</color>

<!-- Home screen -->
<color name="home_show_all_button_text">@color/photonLightGrey50</color>
<color name="home_show_all_button_text">#AB71FF</color>
Copy link
Member

Choose a reason for hiding this comment

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

This should be equivalent to photonViolet40. You can usually see the color name in figma if you select the CSS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Oui, the color variable for photonViolet40 is equivalent to #AB71FF is what I am trying to say
https://searchfox.org/mozilla-mobile/source/android-components/components/ui/colors/src/main/res/values/photon_colors.xml#180

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have that in our .values folder on Fenix. What's the correct way to add a new photon color here?

Copy link
Member

Choose a reason for hiding this comment

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

Should simply be @color/photonViolet40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's in android components. Try it locally. We build our @color/photonXXX values based on https://github.com/mozilla-mobile/android-components/blob/releases/93.0/components/ui/colors/src/main/res/values/photon_colors.xml and you can see that violet40 isn't there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

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'm not adding that in this PR. I'm gonna expose it on AC then update here.

Copy link
Member

Choose a reason for hiding this comment

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

It's there in AC. https://github.com/mozilla-mobile/android-components/blob/main/components/ui/colors/src/main/res/values/photon_colors.xml#L180. We aren't pinned to the 93 release in main which is what you linked.

Trust, I suspect your local dev is not rebased or might need a sync.

Copy link
Member

Choose a reason for hiding this comment

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

We can also omit the change and I can add it in a separate PR if we are still having issues, but the color definitely exists in AC already.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

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

@eliserichards
Copy link
Contributor Author

eliserichards commented Sep 23, 2021

@gabrielluong I cleaned everything up a bunch and removed all of the unnecessary white space around bookmarks and top sites, as well as making sure collections and "no collections" were matching. lmk what you think :)

@eliserichards
Copy link
Contributor Author

Making some changes to "customize" button based on Nicole's feedback...

@eliserichards
Copy link
Contributor Author

eliserichards commented Sep 23, 2021

Modified the collections header so that it takes into account the top margins above each of the collections and still matches the spacing of the other headers. Spaced out the "customize" button so that it matches the space in between each section, even when collections aren't shown:

@mergify
Copy link
Contributor

mergify bot commented Sep 24, 2021

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

@cadeyrn
Copy link
Contributor

cadeyrn commented Sep 24, 2021

@eliserichards While you are at it, could you also fix the overlap of the card headers and the "show all" link which happens in languages with longer translations? I checked out your current work on this issue and while it's better due to the smaller font it's still an issue. I attached a screenshot of the "recently saved" section ("Kürzlich als Lesezeichen gesetzt" in German) so that you can see the issue (left: main branch, right: your branch for this issue).

screenshot

@eliserichards
Copy link
Contributor Author

@cadeyrn thank you so much for pointing that out! I'm going to land this and tackle that issue in a new PR :D

@eliserichards eliserichards added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed pr:do-not-land pr:waiting-for-authors PR that has been approved and awaiting any changes before they can land labels Sep 27, 2021
@eliserichards
Copy link
Contributor Author

Flaky test:

  • TitleHeaderBindingTest. WHEN grouped tabs are added to the list THEN return false

@eliserichards
Copy link
Contributor Author

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2021

Command rebase: success

Branch already up to date

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2021

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

…ns and jump back in spacing

For mozilla-mobile#21339: Add ignores for intermittent ui tests
@eliserichards eliserichards added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed pr:needs-landing PRs that are ready to land [Will be merged by Mergify] labels Sep 28, 2021
@eliserichards eliserichards merged commit 777f2d1 into mozilla-mobile:main Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update "show all" buttons on home
4 participants