Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor FXIOS-7577 [v121] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton #16925

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Refactor FXIOS-7577 [v121] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton #16925

merged 1 commit into from
Oct 23, 2023

Conversation

mingming-ma
Copy link
Contributor

@mingming-ma mingming-ma commented Oct 18, 2023 •
edited

📜 Tickets

Jira ticket
Github issue

💡 Description

This commit is a refactor work that changes the mainButton in FakespotOptInCardView to use PrimaryRoundedButton by the requirements.

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed I updated documentation / comments for complex code and public methods

@mingming-ma mingming-ma requested a review from a team as a code owner October 18, 2023 23:06
@mingming-ma mingming-ma changed the title Refactor FXIOS-16864 [v120] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton Refactor FXIOS-7577 [v120] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton Oct 18, 2023
@mingming-ma
Copy link
Contributor Author

Hi, here are the test screenshots, let me know if any other changes are needed. Thanks!
theme

image2

lmarceau
lmarceau requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@lmarceau lmarceau left a comment

Choose a reason for hiding this comment

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

Well done! What I think we're missing is only the following change:

We can remove the colors being set directly from that view. So removing the lines:

        mainButton.setTitleColor(colors.textInverted, for: .normal)
        mainButton.backgroundColor = colors.actionPrimary

and call instead mainButton.applyTheme(:) directly under the mainButton.configure(viewModel:) that you added. I think that should be it (you can maybe recheck all is good and dark/light mode).

Thank you 🙏

@mingming-ma
Copy link
Contributor Author

@lmarceau Thanks a lot for the review! I found there is no themeManager in the FakespotOptInCardView, so I made a change in FakespotOptInCardViewModel. I tested it and it works, but I'm not quite sure if the change is appropriate. Let me know if any updates are needed. I'll try my best to satisfy. Thanks!

@lmarceau lmarceau changed the title Refactor FXIOS-7577 [v120] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton Refactor FXIOS-7577 [v121] Fakespot - Replace primary button in FakespotOptInCardView to PrimaryRoundedButton Oct 23, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 23, 2023

This pull request has conflicts when rebasing. Could you fix it @mingming-ma? 🙏

@OrlaM OrlaM removed their request for review October 23, 2023 16:42
@mobiletest-ci-bot
Copy link

Messages
📖 Project coverage: 33.41%
📖 Edited 1 files
📖 Created 0 files

Client.app: Coverage: 32.61

File Coverage
FakespotOptInCardView.swift 0.0% ⚠️

Generated by 🚫 Danger Swift against 3dd103f

@lmarceau lmarceau merged commit 9acf5ac into mozilla-mobile:main Oct 23, 2023
9 of 10 checks passed
@thatswinnie
Copy link
Contributor

@Mergifyio backport release/v120

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 9, 2023 •
edited

backport release/v120

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 9, 2023
@mingming-ma @mergify
Refactor FXIOS-7577 [v121] Fakespot - Replace primary button in Fakes…
9ea6554 
…potOptInCardView to PrimaryRoundedButton (#16925)

(cherry picked from commit 9acf5ac)
@mergify mergify bot mentioned this pull request Nov 9, 2023
Merged
thatswinnie pushed a commit that referenced this pull request Nov 9, 2023
@mergify @mingming-ma
Refactor FXIOS-7577 [v121] Fakespot - Replace primary button in Fakes…
a1cac8c 
…potOptInCardView to PrimaryRoundedButton (#16925) (#17237)

(cherry picked from commit 9acf5ac)

Co-authored-by: mingming-ma <133393905+mingming-ma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmarceau lmarceau lmarceau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mingming-ma @mobiletest-ci-bot @thatswinnie @lmarceau