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

Allow the user to set the default EditText color. #3457

Merged
merged 8 commits into from
Mar 19, 2021

Conversation

michelleb-stripe
Copy link
Contributor

Summary

This is a bit of a weird and tricky problem. The StripeEditText wants to set the color of the text based on if there is an error or not. In order to do this it called the setTextColor(Int) for errors and setTextColor(ColorStateList) for non-errors. In order to not replace the existing color it uses two member variables to track the color for error and non-error: errorColor and cachedColorStateList.

In the past the cacheColorState list was initialized by the constructor, and then never changed after that point. However if someone needs to update the color after initialization there was no way to do this. Calling setTextColor would end up just bring replaced as the field was set in error. When put back in a non-error state, it would set the color back to the value on initialization.

Motivation

This will help support customers wanting to set the text color

Testing

  • Added tests
  • Modified tests
  • Manually verified

@@ -89,12 +89,19 @@ open class StripeEditText @JvmOverloads constructor(
maxLines = 1
listenForTextChanges()
listenForDeleteEmpty()
determineDefaultErrorColor()
cachedColorStateList = textColors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't textColors == null before setTextColor() is called?

Comment on lines 98 to 103
override fun setTextColor(colors: ColorStateList?) {
super.setTextColor(colors)

// This will only use textColors and not colors because textColor is never null
cachedColorStateList = textColors
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a new method setDefaultTextColor?

Copy link
Contributor Author

@michelleb-stripe michelleb-stripe Mar 19, 2021

Choose a reason for hiding this comment

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

We could but user's could still call setTextColor? What are the reservations on overriding this method?

Some things I like about this approach is that it works as expected they set the text color, and when there is an error we set it to the error color, and then will return it back to this when done. They don't have to know about the special stuff we are doing. Without changing the inherited parent, there is no way to prevent exposing this to our users.

It is just a function rename, so I can do it either way.

Comment on lines +35 to +37
internal var defaultColorStateList: ColorStateList
@VisibleForTesting
internal set
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove internal set

@michelleb-stripe michelleb-stripe merged commit e017a2d into master Mar 19, 2021
@michelleb-stripe michelleb-stripe deleted the allow-setting-edit-text-color branch March 19, 2021 15:13
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.

2 participants