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.

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

Fix missing theming for add lpm button and notes text #5068

Merged
merged 2 commits into from May 25, 2022

Conversation

skyler-stripe
Copy link
Contributor

Summary

  • Add button is now theme'd correctly
  • Notes text is now theme'd correctly.

Motivation

  • Both compose views are now wrapped in PaymentsTheme, this is best practice anways so that we can mutate the theme during testing. This was a miss while I was refactoring. I'm investigating why the screenshot tests didn't fail on this, I think they could be stricter.

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Before After
ThemeBeforeDark ThemeAfterDark
themeBeforeLight themeAfterLight

Changelog

  • added

@@ -593,131 +599,129 @@ internal fun PaymentOptionUi(
) {
// An attempt was made to not use constraint layout here but it was unsuccessful in
// precisely positioning the check and delete icons to match the mocks.
PaymentsTheme {
ConstraintLayout(
ConstraintLayout(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like I changed way more than I did. I just removed the PaymentsTheme being set.

@skyler-stripe skyler-stripe merged commit 96c7e95 into master May 25, 2022
@skyler-stripe skyler-stripe deleted the materialThemeIssues branch May 25, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants