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

For #8984: Added color for "Share" menu's "Recently Used" in Dark theme #18839

Merged
merged 3 commits into from
May 7, 2021
Merged

For #8984: Added color for "Share" menu's "Recently Used" in Dark theme #18839

merged 3 commits into from
May 7, 2021

Conversation

neha-b2001
Copy link
Collaborator

@neha-b2001 neha-b2001 commented Apr 7, 2021 •
edited
Loading

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

Screenshots:

Before:
image

After:
image

No new accessibility related suggestions came up in Google Accessibility Scanner because of this change.

@neha-b2001 neha-b2001 requested review from a team as code owners April 7, 2021 08:27
@firefoxci-taskcluster
Copy link

No Taskcluster jobs started for this pull request 
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@neha-b2001
Copy link
Collaborator Author

I am an Outreachy applicant, hence tagging @eliserichards. Please take a look if possible.

gabrielluong
gabrielluong suggested changes Apr 7, 2021
View reviewed changes
app/src/main/res/values/colors.xml Outdated
@@ -87,7 +87,7 @@
<color name="contrast_text_dark_theme">@color/primary_text_dark_theme</color>
<color name="caption_text_dark_theme">@color/photonLightGrey70</color>
<color name="foundation_dark_theme">#1C1B22</color>
<color name="inset_dark_theme">#32313C</color>
<color name="inset_dark_theme">#52525E</color>
Copy link
Member

Choose a reason for hiding this comment

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

We have a colour variable for this colour. Can you use @color/photonDarkGrey10?

I think you also do a search and replace where the other instance of #52525E and replace it with the colour variable as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gabrielluong Replaced all instances of #52525E with @color/photonDarkGrey10 👍

@gabrielluong gabrielluong self-assigned this Apr 7, 2021
gabrielluong
gabrielluong approved these changes Apr 8, 2021
View reviewed changes
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.

Looks good, thank you!

@gabrielluong gabrielluong added pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify] labels Apr 8, 2021
@gabrielluong gabrielluong mentioned this pull request Apr 22, 2021
Closed
3 tasks
@gabrielluong gabrielluong added pr:do-not-land and removed pr:needs-landing PRs that are ready to land [Will be merged by Mergify] labels Apr 22, 2021
gabrielluong
gabrielluong previously requested changes Apr 22, 2021
View reviewed changes
app/src/main/res/values/colors.xml Outdated
@@ -87,7 +87,7 @@
<color name="contrast_text_dark_theme">@color/primary_text_dark_theme</color>
<color name="caption_text_dark_theme">@color/photonLightGrey70</color>
<color name="foundation_dark_theme">#1C1B22</color>
<color name="inset_dark_theme">#32313C</color>
<color name="inset_dark_theme">@color/photonDarkGrey10</color>
Copy link
Member

Choose a reason for hiding this comment

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

I think I made an error. We actually need to check all instances of inset_dark_theme and cannot simply change this color. We might want to create a custom variable just for the background here avoid changing all the UI components that might use inset_dark_theme, but was not covered by the original UX review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I'll check the instances of inset_dark_theme 👍

@gabrielluong gabrielluong removed the pr:approved PR that has been approved label Apr 22, 2021
@neha-b2001
Added color for "Share" menu's "Recently Used" in Dark theme
144d10c 
- Changed HEX code for inset_dark_theme in values/colors.xml
@neha-b2001
Replaced #52525E in values/colors.xml with @color/photonDarkGrey10
4028152
@neha-b2001
Added color for "Share" menu's "Recently Used" in Dark theme
7f9165d 
- Created a separate attribute recentlyUsedSharedMenu to be used by 'recently used' panel of tab share menu
- It specifies colors to be used for Light and Dark theme
@neha-b2001
Copy link
Collaborator Author

Hello, @gabrielluong. Actually inset_dark_theme was indeed being used in multiple places throughout the code, especially through inset_normal_theme and search_widget_background, which are using this color.

So, I have created the custom variable recentlyUsedShareMenu, which specifies the light and dark theme colors specifically for this component that we are dealing with. Since recent_apps_background.xml is the shape which determines this background colour, I have changed its color attribute to:
<solid android:color="?recentlyUsedShareMenu"/>

This ensures the same behavior as my previous commit, but without any modification of inset_dark_theme.

@neha-b2001 neha-b2001 requested review from gabrielluong and removed request for a team May 3, 2021 09:51
eliserichards
eliserichards approved these changes May 7, 2021
View reviewed changes
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

This is fantastic. Great work! 🎉

@eliserichards eliserichards mentioned this pull request May 7, 2021
Closed
3 tasks
@gabrielluong gabrielluong added pr:approved PR that has been approved and removed pr:do-not-land labels May 7, 2021
@gabrielluong gabrielluong added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label May 7, 2021
@gabrielluong
Copy link
Member

Let's flag for qa-needed after this lands.

@eliserichards eliserichards dismissed gabrielluong’s stale review May 7, 2021 19:48

Addressed comments :)

@eliserichards eliserichards merged commit 0eaf9f3 into mozilla-mobile:master May 7, 2021
@neha-b2001
Copy link
Collaborator Author

This is fantastic. Great work! 🎉

Thank you! 😄

@neha-b2001 neha-b2001 deleted the recently-used-dark-theme branch June 6, 2021 04:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@topotropic topotropic topotropic approved these changes

@eliserichards eliserichards eliserichards approved these changes

@gabrielluong gabrielluong Awaiting requested review from gabrielluong

Assignees

@topotropic topotropic

@gabrielluong gabrielluong

Labels
pr:approved PR that has been approved pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] "Share" menu "Recently Used" is not colored in Dark theme
4 participants
@neha-b2001 @gabrielluong @topotropic @eliserichards