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

For #27468 - Backplate homepage MessageCard #27677

Merged

Conversation

MozillaNoah
Copy link
Contributor

@MozillaNoah MozillaNoah commented Nov 2, 2022

Light Dark

In essence, this section is styled the same as the recent synced tab (Jump back in)

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.

QA

  • QA Needed

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

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-debug task.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Fixes #27468

@MozillaNoah MozillaNoah added needs:review PRs that need to be reviewed Needs-UX Issues or tickets that need UX input or review labels Nov 2, 2022
@MozillaNoah MozillaNoah requested review from a team as code owners November 2, 2022 22:48
Comment on lines 60 to 63
val defaultMessageCardColors = MessageCardColors.buildMessageCardColors()

var buttonColor = defaultMessageCardColors.buttonColor
var buttonTextColor = defaultMessageCardColors.buttonTextColor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val defaultMessageCardColors = MessageCardColors.buildMessageCardColors()
var buttonColor = defaultMessageCardColors.buttonColor
var buttonTextColor = defaultMessageCardColors.buttonTextColor
var (buttonColor, buttonTextColor) = MessageCardColors.buildMessageCardColors()

Wondering if we could use destructuring declaration here to save us a few lines https://kotlinlang.org/docs/destructuring-declarations.html

Copy link
Contributor

@MatthewTighe MatthewTighe Nov 3, 2022

Choose a reason for hiding this comment

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

If we wanted to go fully immutable you may be able to do something like this, even:

val (buttonColor, buttonTextColor) = if (isWallpaperNotDefault) {
    FirefoxTheme.colors.layer1 to if (!isSystemInDarkTheme()) { FirefoxTheme.colors.textActionSecondary } else defaultMessageCardColors.buttonTextColor
} else {
   MessageCardColors.buildMessageCardColors()
}

Definitely a little code golfy though 😅

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 had no idea about the destructuring! that's really cool! I had to just insert some underscores though, as the destructuring is tied to the ordering of the parameters.

var (_, _, _, _, buttonColor, buttonTextColor) = MessageCardColors.buildMessageCardColors()

@gabrielluong gabrielluong added pr:approved PR that has been approved and removed needs:review PRs that need to be reviewed labels Nov 3, 2022
@MozillaNoah MozillaNoah force-pushed the backplate-homepage-message-card branch from d4b8d83 to 1be535c Compare November 3, 2022 16:55
@Kate-Galetski
Copy link

Looks Correct!

@MozillaNoah MozillaNoah added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed Needs-UX Issues or tickets that need UX input or review labels Nov 3, 2022
@mergify mergify bot merged commit 07e4613 into mozilla-mobile:main Nov 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

Wallpapers - Adapt "set as default browser" button with wallpaper card color treatment
4 participants